Merge lp://staging/~freyes/charms/trusty/memcached/python-rewrite into lp://staging/charms/trusty/memcached

Proposed by Felipe Reyes
Status: Superseded
Proposed branch: lp://staging/~freyes/charms/trusty/memcached/python-rewrite
Merge into: lp://staging/charms/trusty/memcached
Diff against target: 4436 lines (+3861/-264)
39 files modified
.bzrignore (+5/-0)
Makefile (+39/-0)
charm-helpers.yaml (+6/-0)
config.yaml (+6/-6)
hooks/cache-relation-joined (+0/-9)
hooks/charmhelpers/contrib/network/ip.py (+351/-0)
hooks/charmhelpers/contrib/network/ovs/__init__.py (+80/-0)
hooks/charmhelpers/contrib/network/ufw.py (+189/-0)
hooks/charmhelpers/core/fstab.py (+118/-0)
hooks/charmhelpers/core/hookenv.py (+542/-0)
hooks/charmhelpers/core/host.py (+416/-0)
hooks/charmhelpers/core/services/__init__.py (+2/-0)
hooks/charmhelpers/core/services/base.py (+313/-0)
hooks/charmhelpers/core/services/helpers.py (+243/-0)
hooks/charmhelpers/core/sysctl.py (+34/-0)
hooks/charmhelpers/core/templating.py (+52/-0)
hooks/charmhelpers/fetch/__init__.py (+416/-0)
hooks/charmhelpers/fetch/archiveurl.py (+145/-0)
hooks/charmhelpers/fetch/bzrurl.py (+54/-0)
hooks/charmhelpers/fetch/giturl.py (+51/-0)
hooks/config-changed (+0/-155)
hooks/install (+0/-20)
hooks/memcached_hooks.py (+217/-0)
hooks/memcached_utils.py (+27/-0)
hooks/munin-relation-changed (+0/-29)
hooks/nrpe-external-master-relation-changed (+0/-19)
hooks/start (+0/-2)
hooks/stop (+0/-3)
hooks/upgrade-charm (+0/-4)
metadata.yaml (+2/-2)
templates/memcached.conf (+62/-0)
templates/munin-node.conf (+66/-0)
templates/nrpe_check_memcached.cfg.tmpl (+1/-1)
templates/nrpe_export.cfg.tmpl (+3/-3)
test_requirements.txt (+7/-0)
tests/10_deploy_test.py (+22/-11)
unit_tests/__init__.py (+2/-0)
unit_tests/test_memcached_hooks.py (+268/-0)
unit_tests/test_utils.py (+122/-0)
To merge this branch: bzr merge lp://staging/~freyes/charms/trusty/memcached/python-rewrite
Reviewer Review Type Date Requested Status
Jorge Niedbalski (community) Approve
Review via email: mp+244232@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-12-09.

This proposal has been superseded by a proposal from 2014-12-09.

Description of the change

Dear Charmers,

This MP is a rewrite in python to leverage charmhelpers and secure memcached service using ufw rules to only allow access from machines related to the service.

Summary of changes:

* Drop all the bash in favor of python
* Added unit tests
* Changed config type to boolean of following keys:
  * disable-auto-cleanup
  * disable-cas
  * disable-large-pages
* Use ufw to secure memcached[0]

[0] root@ubuntu:~# ufw status # before relating memcache with mediawiki
Status: active

To Action From
-- ------ ----
22 ALLOW Anywhere
4949 ALLOW Anywhere
22 (v6) ALLOW Anywhere (v6)
4949 (v6) ALLOW Anywhere (v6)
21/tcp ALLOW ::1

root@ubuntu:~# ufw status # after juju add-relation memcached mediawiki
Status: active

To Action From
-- ------ ----
22 ALLOW Anywhere
4949 ALLOW Anywhere
11211/tcp ALLOW 192.168.0.120
22 (v6) ALLOW Anywhere (v6)
4949 (v6) ALLOW Anywhere (v6)

To post a comment you must log in.
Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

Hello Felipe,

Thanks for this contribution. I made some inline comments, required to be fixed. Also i have a few suggestions for improving.

On the lint side,

- Remove the 'tags' section on the metadata.yaml file.
- Add the categories: ["system"] on the metadata.yaml file.

On the test side,

- Remove the pre_install_hooks code.
- Add test coverage for the method cache_relation_departed
- Add test coverage for the method upgrade_charm

On the memcache_utils file.

- Please instead of create your own template rendering method, please see:
hooks/charmhelpers/core/templating.py
- Add test coverage for method revoke_access

review: Needs Fixing
Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

Hello Felipe,

The 'make test' target doesn't works properly, in fact, it references to files that doesn't longer exists on the project.

Please fix them and OTT LGTM

review: Needs Fixing
Revision history for this message
Felipe Reyes (freyes) wrote : Posted in a previous version of this proposal

@niedbalski, I fixed the amulet test, please try running 'make test' again. Thanks.

Revision history for this message
Review Queue (review-queue) wrote : Posted in a previous version of this proposal

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10637-results

review: Needs Fixing (automated testing)
Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

Felipe,

Your makefile is assuming you already have nosetests installed,
Could you please modify your Makefile for making it looking like this one https://pastebin.canonical.com/122033/ ?

review: Needs Fixing
93. By Felipe Reyes

Add flake8 to test_requirements

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

Felipe,

Everything looks OK. Tests/Lint passing and you followed the overall suggestions.

Thanks

review: Approve
Revision history for this message
Review Queue (review-queue) wrote : Posted in a previous version of this proposal

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10646-results

review: Needs Fixing (automated testing)

Unmerged revisions

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