Juju Charms Collection
cassandra package

Merge lp://staging/~ev/charms/xenial/cassandra/snap into lp://staging/~cassandra-charmers/charms/xenial/cassandra/trunk

  1. Xenial Xerus (16.04)
  2. snap
  3. Merge into trunk
Proposed by Evan
Status: Merged
Merge reported by: Stuart Bishop
Merged at revision: not available
Proposed branch: lp://staging/~ev/charms/xenial/cassandra/snap
Merge into: lp://staging/~cassandra-charmers/charms/xenial/cassandra/trunk
Diff against target: 498 lines (+224/-31)
4 files modified
config.yaml (+4/-4)
hooks/actions.py (+10/-2)
hooks/helpers.py (+167/-24)
tests/test_helpers.py (+43/-1)
To merge this branch: bzr merge lp://staging/~ev/charms/xenial/cassandra/snap
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+297723@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Comments inline.

Revision history for this message
Evan (ev) wrote :

Thanks for the thorough review! I've fixed the raised issues and mentioned the individual revnos inline.

Revision history for this message
Stuart Bishop (stub) wrote :

This is all looking good. A few comments inline, but nothing series except for the encrypt() method failing under trusty.

We will want integration tests, which starts with creating TestSnapDeployment(Test3UnitDeployment) in tests/test_integration.py. The tests and harness on this branch are Juju1 specific (I can sort Juju 2.0 mostly by syncing testing/amuletfixture.py to the PostgreSQL charm's version).

The integration tests at the moment will almost certainly fail for snap, as they 'juju run --unit ... nodetool ...' so we either need the charm to create symlinks in /usr/local/bin so the standard tool names work, or update the callsites in the test suite (just 3 or 4 I see). Ideally the snap would use the standard tool names like 'nodetool' rather than 'cassandra.nodetool', but from what I can tell that would mean maintaining a separate snap for each cli tool?

Revision history for this message
Evan (ev) :
lp://staging/~ev/charms/xenial/cassandra/snap updated
390. By Evan

Add bug reference.

391. By Evan

Correct default for edition.

392. By Evan

Prefer platform.dist to host.lsb_release

393. By Evan

Do not try to decode from a string

394. By Evan

Fix tests; reset log mocks between calls.

395. By Evan

Revert move from lsb_release to platform: https://bugs.python.org/issue1322#msg263313

Revision history for this message
Evan (ev) :
Revision history for this message
Stuart Bishop (stub) wrote :

I think this is past the point where it can land. I'm running tests.

review: Approve
Revision history for this message
Stuart Bishop (stub) wrote :

I needed to make some more changes, which at at lp:~stub/charms/trusty/cassandra/xenial. The charm is now multi-series, and I'm pushing it to a new home at https://launchpad.net/cassandra-charm ( git+ssh://git.launchpad.net/cassandra-charm ). Further work should land on the git branch (let me know if you have much outstanding work that can't be easily moved in).

There are no integration tests of the snap install yet. Which is fine, as there is no documentation for the new feature either :-) We should fix both of these issues on the git branch.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: