Juju Charms Collection
bip package

Merge lp://staging/~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894 into lp://staging/charms/trusty/bip

  1. Trusty Tahr (14.04)
  2. client_side_ssl-with_helper-lp1604894
  3. Merge into trunk
Proposed by Jose L. VG
Status: Needs review
Proposed branch: lp://staging/~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894
Merge into: lp://staging/charms/trusty/bip
Prerequisite: lp://staging/~josvaz/charms/trusty/bip/charmhelpers-cleanup
Diff against target: 618 lines (+496/-7)
8 files modified
charm-helpers.yaml (+1/-0)
config.yaml (+4/-0)
hooks/hooks.py (+30/-6)
lib/charmhelpers/contrib/__init__.py (+13/-0)
lib/charmhelpers/contrib/ssl/__init__.py (+92/-0)
lib/charmhelpers/contrib/ssl/service.py (+277/-0)
templates/bip_conf.template (+1/-1)
tests/11-deploy-ssl (+78/-0)
To merge this branch: bzr merge lp://staging/~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894
Reviewer Review Type Date Requested Status
Review Queue (community) automated testing Approve
Brad Marshall (community) Approve
Pen Gale (community) Approve
charmers Pending
Review via email: mp+301802@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Jose L. VG (josvaz) wrote :

This has been tested on a juju setup on GCE

Reply
Revision history for this message
Pen Gale (pengale) wrote :

Code looks sensible, and does not break existing tests, but I would like to see unit or integration tests that cover the self signing code.

review: Needs Fixing
Reply
Revision history for this message
Jose L. VG (josvaz) wrote :

Would a test that calls gen_certs() and verifies the generated pem files are for a self signed cert & key suffice?

Reply
Revision history for this message
Jose L. VG (josvaz) wrote :

Also this MP depends on this other to be done first:
https://code.launchpad.net/~josvaz/charms/trusty/bip/charmhelpers-cleanup/+merge/301499

Do I need to fix anything in that one?

Reply
lp://staging/~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894 updated
32. By Jose L. VG

Adding a bip ssl deploy test (validates cert & handshake)

Revision history for this message
Jose L. VG (josvaz) wrote :

Added a 11-deploy-ssl test that deploys a ssl enabled bip on port 6697, it retrieves the server certificate and then validates against it a SSL connection.

I guess this covers all the new feature code.

Please review!

Reply
Revision history for this message
Pen Gale (pengale) wrote :

Hello Jose,

Thank you for all your work on this. The new test looks great, and passes when I run bundletester. :-)

I am +1 on this.

There is an outstanding issue with merging and promulgating this, which I noted in the related PR: the bip charm is currently own by ~charmers, rather than a specific maintainer. I can't fix that myself, but I am poking people about it; I'll ping this ticket when the issue is fixed.

review: Approve
Reply
Revision history for this message
Pen Gale (pengale) wrote :

Dropping in an update here. The next steps for this change need to be:

@bradm needs to update his branch of this charm with the latest from ~charmers, then merge this PR into it. Then he needs to promulgate a charm from his namespace, and request that a charmer merge it.

That should update the charm store so that bip gets pointed at bradm's namespace (since he's the maintainer), and should allow bradm to more easily maintain the charm going forward.

@josvaz: you most likely don't need to do anything else with this PR; the maintainer (bradm) just needs to do some housekeeping to get things merged.

Reply
Revision history for this message
Jose L. VG (josvaz) wrote :

Thanks fo rthe update Pete!

Jose

On Thu, Aug 25, 2016 at 3:43 PM, petevg <email address hidden>
wrote:

> Dropping in an update here. The next steps for this change need to be:
>
> @bradm needs to update his branch of this charm with the latest from
> ~charmers, then merge this PR into it. Then he needs to promulgate a charm
> from his namespace, and request that a charmer merge it.
>
> That should update the charm store so that bip gets pointed at bradm's
> namespace (since he's the maintainer), and should allow bradm to more
> easily maintain the charm going forward.
>
> @josvaz: you most likely don't need to do anything else with this PR; the
> maintainer (bradm) just needs to do some housekeeping to get things merged.
> --
> https://code.launchpad.net/~josvaz/charms/trusty/bip/
> client_side_ssl-with_helper-lp1604894/+merge/301802
> You are the owner of lp:~josvaz/charms/trusty/bip/
> client_side_ssl-with_helper-lp1604894.
>

Reply
Revision history for this message
Pen Gale (pengale) wrote :

Poked bradm about confirming that bip is moved and closing this review out.

Reply
Revision history for this message
Brad Marshall (brad-marshall) wrote :

Looks good to me, merged into my own branch and am working on getting it updated in the charmstore.

review: Approve
Reply
Revision history for this message
Jose L. VG (josvaz) wrote :

Thanks Brad, let me know if/when this MP needs to be marked Merged manually.

Reply
Revision history for this message
Review Queue (review-queue) wrote :

The results (PASS) are in and available here: http://juju-ci.vapour.ws/job/charm-bundle-test-aws/5067/

review: Approve (automated testing)
Reply
Revision history for this message
Review Queue (review-queue) wrote :

The results (PASS) are in and available here: http://juju-ci.vapour.ws/job/charm-bundle-test-lxc/4866/

review: Approve (automated testing)
Reply

Unmerged revisions

32. By Jose L. VG

Adding a bip ssl deploy test (validates cert & handshake)

31. By Jose L. VG

Added support to client_side_ssl with charmhelper contrib.ssl

30. By Jose L. VG

Got charmhelpers for fetch as apt_install is now there

29. By Jose L. VG

Moved from charm-helpers/charmhelpers to just charmhelpers (plus clean-up)

28. By Jose L. VG

Result of removing charmhelpers and re-syncing it

27. By Jose L. VG

Added charm-helpers.yaml to manage helpers with charm_helpers_sync tool

26. By Jose L. VG

Flake8 suggested fixes, including unused imports

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: