Merge lp:~canonical-is-sa/charms/trusty/kibana/trunk into lp:charms/trusty/kibana

Proposed by Michael Foley
Status: Rejected
Rejected by: Michael Foley
Proposed branch: lp:~canonical-is-sa/charms/trusty/kibana/trunk
Merge into: lp:charms/trusty/kibana
Diff against target: 82 lines (+13/-10)
4 files modified
config.yaml (+4/-0)
files/charm/config.js (+1/-1)
hooks/install (+8/-0)
hooks/web-relation-joined (+0/-9)
To merge this branch: bzr merge lp:~canonical-is-sa/charms/trusty/kibana/trunk
Reviewer Review Type Date Requested Status
Michael Foley (community) superseded Disapprove
Andrew McLeod (community) Needs Fixing
charmers Pending
Adam Israel Pending
Review via email: mp+276221@code.launchpad.net

This proposal supersedes a proposal from 2015-05-25.

Description of the change

Added ability to change the elasticsearch url for kibana so that it can be set to https. This is so that kibana can be secured behind an apache2 proxy with openid.

Also added support for ubuntu basenode.

To post a comment you must log in.
Revision history for this message
Adam Israel (aisrael) wrote : Posted in a previous version of this proposal

Hi Michael,

I had a chance to review this merge proposal today. Thanks for your work on improving the kibana charm!

`charm proof` alerted me to 'hooks/website-relation-joined' not being executable. That's not a blocker, as juju should do the right thing, but is an easy fix.

With a new feature such as this, I look for two things that I'm not seeing: some documentation about the new feature in the README, and a test to exercise it. If you could address both of those issues, I'll happily approve this merge.

Thanks once again for your work on this!

review: Needs Fixing
Revision history for this message
Michael Nelson (michael.nelson) wrote : Posted in a previous version of this proposal

Hey there Michael. I'm just looking at this charm change, and it looks like upstream has already changed the web relation to 'http'. So I think that part is done. There are two other features you've included - the elasticsearch url config option, and the exec.d support.

I'll give you an MP to merge into your canonical-is-sa branch which re-merges trunk, so we can push those two options up for review again.

Revision history for this message
Andrew McLeod (admcleod) wrote :

Running bundletester against this hits some issues that are fixed by https://code.launchpad.net/~chris.macnaughton/charms/trusty/kibana/version_bump/+merge/274029 which has been approved and merged. Could you please pull these changes in and someone can test?

review: Needs Fixing
Revision history for this message
Michael Nelson (michael.nelson) wrote :

If you mean that tests/11-scale-elastic.py fails, that would be a good thing IMO, as we updated our branch here to explicitly deny requests to :9200 on the kibana unit (as it's basically giving anyone with access to :80 on the kibana unit write access to the ES cluster).

Anyway, I've re-merged upstream, and updated 11-scale-elastic.py so it doesn't expect to be able to use :9200 on the kibana unit externally, in our kibana4 branch here:
https://code.launchpad.net/~canonical-is-sa/charms/trusty/kibana/kibana4/+merge/276944

I think this MP should be marked superseded. Michael F?

Revision history for this message
Michael Foley (foli) wrote :

Agreed it should be marked superseded.

The elasticsearch url setting is no longer needed with kibana4. So the only actual thing left in the MP is the adding of ubuntu basenode (exec.d), which looks like is covered in Michael N's MP.

Marking this MP as rejected instead of deleting it since it is linked to in a few places.

review: Disapprove (superseded)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config.yaml'
2--- config.yaml 2015-04-01 20:07:40 +0000
3+++ config.yaml 2015-10-30 07:18:46 +0000
4@@ -20,3 +20,7 @@
5 default: ''
6 description: 'The url path of kibana. This is required if kibana is
7 proxied at a non root path. e.g. http://example.com/kibana/'
8+ elasticsearch_url:
9+ type: string
10+ default: '"http://"+window.location.hostname'
11+ description: URL to for elasticsearch line in config.sh, defaults to '"http://"+window.location.hostname'
12
13=== modified file 'files/charm/config.js'
14--- files/charm/config.js 2014-04-14 15:59:48 +0000
15+++ files/charm/config.js 2015-10-30 07:18:46 +0000
16@@ -72,4 +72,4 @@
17 'sparklines'
18 ]
19 });
20-});
21\ No newline at end of file
22+});
23
24=== modified file 'hooks/config-changed' (properties changed: -x to +x)
25=== modified file 'hooks/install'
26--- hooks/install 2015-06-18 18:49:22 +0000
27+++ hooks/install 2015-10-30 07:18:46 +0000
28@@ -1,10 +1,15 @@
29 #!/bin/bash
30 set -eux
31+
32+juju-log 'Invoking charm-pre-install hooks'
33+[ -d exec.d ] && ( for f in exec.d/*/charm-pre-install; do [ -x $f ] && /bin/sh -c "$f"; done )
34+
35 HOME=$PWD
36 KIBANA_TARBALL=files/kibana.tgz
37 KIBANA_SOURCE=$(config-get kibana_source)
38 KIBANA_SOURCE_UNPACK_DIRECTORY=$(config-get kibana_source_unpack_directory)
39 KIBANA_SOURCE_CHECKSUM=$(config-get kibana_source_checksum)
40+ELASTICSEARCH_URL=$(config-get elasticsearch_url)
41 HOST=$(unit-get private-address)
42
43 juju-log "install dependency"
44@@ -38,6 +43,9 @@
45 rm -f /srv/kibana3/config.js
46 install -o root -g root -m 0644 files/charm/config.js /srv/kibana3/config.js
47
48+# edit the elasticsearch line to be the value of option elasticsearch_url
49+sed -r -i -e 's|(^\s*elasticsearch:\s*).*$|\1'"${ELASTICSEARCH_URL}"',|' /srv/kibana3/config.js
50+
51 hooks/config-changed
52
53 ln -s /etc/nginx/sites-available/kibana /etc/nginx/sites-enabled/kibana
54
55=== modified file 'hooks/start' (properties changed: -x to +x)
56=== added file 'hooks/web-relation-joined'
57--- hooks/web-relation-joined 1970-01-01 00:00:00 +0000
58+++ hooks/web-relation-joined 2015-10-30 07:18:46 +0000
59@@ -0,0 +1,9 @@
60+#!/bin/bash
61+set -eux
62+juju-log $JUJU_REMOTE_UNIT joined
63+
64+PORT=$(config-get port)
65+
66+# Set ip/port for web relation
67+relation-set port="$PORT"
68+relation-set host=`unit-get private-address`
69
70=== removed file 'hooks/web-relation-joined'
71--- hooks/web-relation-joined 2015-04-01 20:07:40 +0000
72+++ hooks/web-relation-joined 1970-01-01 00:00:00 +0000
73@@ -1,9 +0,0 @@
74-#!/bin/bash
75-set -eux
76-juju-log $JUJU_REMOTE_UNIT joined
77-
78-PORT=$(config-get port)
79-
80-# Set ip/port for web relation
81-relation-set port="$PORT"
82-relation-set host=`unit-get private-address`

Subscribers

People subscribed via source and target branches