Merge lp:~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894 into lp:charms/trusty/bip
| Status: | Needs review | ||||
|---|---|---|---|---|---|
| Proposed branch: | lp:~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894 | ||||
| Merge into: | lp:charms/trusty/bip | ||||
| Prerequisite: | lp:~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:~josvaz/charms/trusty/bip/client_side_ssl-with_helper-lp1604894 | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Review Queue (community) | automated testing | Approve on 2017-09-25 | |
| Brad Marshall (community) | Approve on 2016-12-05 | ||
| Pete Vander Giessen | Approve on 2016-08-19 | ||
| charmers | 2016-08-02 | Pending | |
|
Review via email:
|
|||
Description of the Change
This should replace https:/
It goes after this MP:
https:/
And fixes:
https:/
X-CPC-Summary-Skip: 1
| Jose L. VG (josvaz) wrote : | # |
| Pete Vander Giessen (petevg) 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.
| 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?
| Jose L. VG (josvaz) wrote : | # |
Also this MP depends on this other to be done first:
https:/
Do I need to fix anything in that one?
- 32. By Jose L. VG on 2016-08-19
-
Adding a bip ssl deploy test (validates cert & handshake)
| 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!
| Pete Vander Giessen (petevg) 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.
| Pete Vander Giessen (petevg) 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.
| 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:/
> client_
> You are the owner of lp:~josvaz/charms/trusty/bip/
> client_
>
| Pete Vander Giessen (petevg) wrote : | # |
Poked bradm about confirming that bip is moved and closing this review out.
| Brad Marshall (brad-marshall) wrote : | # |
Looks good to me, merged into my own branch and am working on getting it updated in the charmstore.
| Jose L. VG (josvaz) wrote : | # |
Thanks Brad, let me know if/when this MP needs to be marked Merged manually.
| Review Queue (review-queue) wrote : | # |
The results (PASS) are in and available here: http://
| Review Queue (review-queue) wrote : | # |
The results (PASS) are in and available here: http://
Unmerged revisions
- 32. By Jose L. VG on 2016-08-19
-
Adding a bip ssl deploy test (validates cert & handshake)
- 31. By Jose L. VG on 2016-08-02
-
Added support to client_side_ssl with charmhelper contrib.ssl
- 30. By Jose L. VG on 2016-07-29
-
Got charmhelpers for fetch as apt_install is now there
- 29. By Jose L. VG on 2016-07-29
-
Moved from charm-helpers/
charmhelpers to just charmhelpers (plus clean-up) - 28. By Jose L. VG on 2016-07-29
-
Result of removing charmhelpers and re-syncing it
- 27. By Jose L. VG on 2016-07-29
-
Added charm-helpers.yaml to manage helpers with charm_helpers_sync tool
- 26. By Jose L. VG on 2016-07-29
-
Flake8 suggested fixes, including unused imports

This has been tested on a juju setup on GCE