Code review comment for lp:~hopem/charms/trusty/jenkins/python-redux

Revision history for this message
Whit Morriss (whitmo) wrote :

Thanks Edward, your python rewrite generally looks good at a glance.

I confirmed the test failures reported by automated testing: jenkins-slave is not being found because it is in the precise series only.

Changing test/100-deploy-trusty:line 19 to "d.add('jenkins-slave', 'cs:precise/jenkins-slave')" remedies this issue until there is a trusty version of jenkins slave.

The tests also hit an error in "master-relation-changed" due to what appears to be a race condition with the configuration and restart of the jenkins slave and adding a node to master. Running the hook via debug hook runs fine.

See logs: https://gist.githubusercontent.com/anonymous/0067138ce2cc697b8c88/raw/3f4c03688400b32f3aa66e1bd3bad5b7398f80a5/jenkins-race-condition

I confirmed this as an issue in the merge targets bash implementation also. Adding some retry logic to add_node should fix this issues.

-1 for test fixes, but otherwise looks good. Thanks again!

review: Needs Fixing

« Back to merge proposal