Juju Charms Collection
squid-reverseproxy package

Merge lp://staging/~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix into lp://staging/charms/trusty/squid-reverseproxy

  1. Trusty Tahr (14.04)
  2. squid-reverseproxy-nrpe-fix
  3. Merge into trunk
Proposed by Jacek Nykis
Status: Merged
Merged at revision: 51
Proposed branch: lp://staging/~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix
Merge into: lp://staging/charms/trusty/squid-reverseproxy
Diff against target: 183 lines (+35/-5)
3 files modified
files/check_squidpeers (+6/-1)
hooks/hooks.py (+1/-1)
hooks/tests/test_nrpe_hooks.py (+28/-3)
To merge this branch: bzr merge lp://staging/~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Jacek Nykis (community) Needs Resubmitting
Tim Van Steenburgh (community) Needs Fixing
Review Queue (community) automated testing Needs Fixing
Review via email: mp+245312@code.staging.launchpad.net

Description of the change

This change fixes check_squidpeers nrpe check which does not work with non default port

To post a comment you must log in.
Revision history for this message
Review Queue (review-queue) wrote :

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

review: Needs Fixing (automated testing)
Revision history for this message
Jacek Nykis (jacekn) wrote :

Sorry I am not sure what the problem is here. I have not touched the line that is failing at all.

All tests pass on my laptop. I am also running the charm in my environment and it's completely fine.

review: Needs Resubmitting
Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote :

The problem is that the tests depend on the python-apt package, but the `make test` target does not ensure that it's installed.

review: Needs Fixing
Revision history for this message
Jacek Nykis (jacekn) wrote :

Hi Tim,

Understood but how does this relate to my change? I have not touched the piece of code that fails. I am almost certain this is pre existing problem and my change is irrelevant here.

If this MP needs to depend on the Makefile fix so be it but I believe it should be separate piece of work.
When I have time I may look at fixing the Makefile but at this point I have different priorities.

review: Needs Resubmitting
Revision history for this message
Jacek Nykis (jacekn) wrote :

Hello,

It's been 2 weeks since my last comment. Is there anything else you need from me to merge this bugfix into the charmstore charm?

Revision history for this message
Charles Butler (lazypower) wrote :

Hi Jacek,

Thanks for the improvement to the squid-reverse-proxy charm! It appears that there was some back and forth regarding dependency management in the charm regarding CI. I've reviewed the charm based on the merits of the MP, as we are tracking that issue independently:

https://bugs.launchpad.net/charms/+source/squid-reverseproxy/+bug/1401323

If you happen to have the time to address this as another branch that would be brilliant!

The changes contained herein look good to me and will be merged upstream. Thanks for your patience and dedication during the charm review process.

All the best!

review: Approve

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: