Merge lp://staging/~evarlast/charms/trusty/kibana/add-port-path-config into lp://staging/charms/trusty/kibana
Proposed by
Jay R. Wren
| Status: | Merged |
|---|---|
| Merged at revision: | 18 |
| Proposed branch: | lp://staging/~evarlast/charms/trusty/kibana/add-port-path-config |
| Merge into: | lp://staging/charms/trusty/kibana |
| Diff against target: |
153 lines (+72/-9) 8 files modified
config.yaml (+9/-0) hooks/config-changed (+18/-0) hooks/install (+5/-2) hooks/start (+1/-3) hooks/stop (+3/-2) hooks/web-relation-joined (+3/-1) metadata.yaml (+1/-1) tests/12-port-change.py (+32/-0) |
| To merge this branch: | bzr merge lp://staging/~evarlast/charms/trusty/kibana/add-port-path-config |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jay R. Wren (community) | Needs Resubmitting | ||
| Whit Morriss (community) | Needs Fixing | ||
| Michael Foley (community) | Needs Fixing | ||
| Charles Butler (community) | Needs Fixing | ||
|
Review via email:
|
|||
Description of the change
Adds nginx tcp port configuration option and a path option for configuring it to be proxy fronted with a path. e.g. /kibana and not at the root of a url.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Greetings Jay,
Thanks for this contribution. I've taken some time to look this over, and I have a few comments:
There's nothing that stands out about the code - however it would be nice to have the tests updated to flex the configuration option and validate that the resulting service is available on the port/path specified with the new configuration options. This will prevent us from breaking it in the future.
I did however notice that the charm when deployed via the tests results in failure. You can view the results here: http://
The Install hook is the culprit for the deployment failure. DEBUG:runner: unit: kibana/0: machine: 1 agent-state: error details: hook failed: "install" So I cannot block on this - but I will block on the lack of test coverage of the new options.
I'm happy to work with you on updating the tests if you need assistance to cover the new options. Feel free to reach out via IRC.
Otherwise, this MP looks good and aside from the minor feedback above is nearly ready for inclusion in the charm.
Thanks again for the submission. I'm going to change status of this MP to "needs work" and when you're ready switch merge status to 'needs review' and someone will be along shortly to review your work.
If you have any questions/