Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Moving bundleservice charm to reactive #2
Conversation
|
Test FAILed. |
|
Test FAILed. |
makyo
reviewed
Aug 29, 2016
| - default: 8084 | ||
| - description: This is the tcp port on which the bundleservices server will listen for connections. | ||
| + default: 8000 | ||
| + description: (READ ONLY) This is the tcp port on which the bundleservices server will listen for connections. |
makyo
Aug 29, 2016
Member
bundleservice doesn't yet know how to accept a port/config yaml, but may in the future.
hatched
reviewed
Aug 29, 2016
| @@ -0,0 +1,6 @@ | ||
| +includes: ['layer:basic', 'layer:apt', 'layer:nagios', 'interface:nrpe-external-master'] | ||
| +repo: git+ssh://git@github.com:makyo/bundleservice-charm-reactive.git |
|
This is awesome thanks for sticking with this @makyo If I could make a couple suggestions. In a follow-up we should add some tests for the nrpe relation and the config changed hook. Also the readme is a little out of date now so in a follow-up it could use some work. |
|
Test FAILed. |
|
Test FAILed. |
bac
reviewed
Aug 29, 2016
| @@ -1,4 +1,7 @@ | ||
| name: bundleservice | ||
| +series: | ||
| + - trusty | ||
| + - xenial | ||
| summary: provide bundle information | ||
| maintainer: Juju GUI Team <juju-gui@lists.launchpad.net> | ||
| description: Juju Bundle Service provides information about bundles. |
bac
Aug 29, 2016
Collaborator
This could be beefed up. What kind of information and to who for what purposes?
bac
reviewed
Aug 29, 2016
| + | ||
| +sudo add-apt-repository ppa:juju/stable -y | ||
| +sudo apt-get update | ||
| +sudo apt-get install amulet python-requests -y |
bac
Aug 29, 2016
Collaborator
Are these two test files mainly boilerplate? They are inconsistent.
10-deploy has python3 on the shebang. 00-setup installs python-requests not python3-requests, which I just verified is required for trusty.
bac
reviewed
Aug 29, 2016
| + | ||
| + def test_service(self): | ||
| + # test we can access over http | ||
| + page = requests.get('http://{}'.format(self.unit.info['public-address'])) |
bac
Aug 29, 2016
Collaborator
Is port 8000 part of 'public-address'? Where does it come from? It would be a better test to see it responding on port 8000 since that is what is hard-coded in the charm.
|
Great start to the layered charm @makyo. It'll be great if we get it all nailed down and it becomes a pattern for our other charms for go-based services. |
bac
added
+1
QA in progress
labels
Aug 29, 2016
|
The README is out of date. It includes which the charm does not currently support. |
|
QA bad at the moment, pending resolution of the issue where the nrpe charm is only available for precise. Any insight appreciated. |
bac
removed
the
+1
label
Aug 31, 2016
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
bac
reviewed
Sep 5, 2016
| + | ||
| +# Since having a Makefile in the layer means that the Makefile that would wind | ||
| +# up in the built charm gets overwritten, the generated Makefile is included | ||
| +# here so that the targets remain. |
bac
reviewed
Sep 5, 2016
| + | ||
| +@when('config.changed') | ||
| +def config_changed(): | ||
| + restart() |
bac
Sep 5, 2016
Collaborator
I know you have a READ-ONLY caveat about listen-port, but when changing the port is supported I think here you should remove_state('bundleservice-charm.nrpe-check-added') to free up setup_nagios to run again.
|
Once you sort out the CI issues I think this initial version is ready to land. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
BGB here we go |
makyo commentedAug 29, 2016
No description provided.