Code review comment for lp:~fginther/charms/trusty/jenkaas/configure-slaves

Revision history for this message
Francis Ginther (fginther) wrote :

> Looks good except an inline comment about an unused import.

Thanks. Ran flake8 and resolved 2 issues including the unused import.

> A little bit curious about if we could not use 'start' callbacks in the first
> ServiceManager instance for creating the slave. By then the service will have
> started, i guess.

Adding 'start' callbacks would be a way to make sure configure_slaves only runs after the jenkins service is started, but there would have to be a delay between the two as it takes jenkins a few seconds to be responsive. If this becomes a problem for the slave_manager, it could be addressed by adding a 'required_data' callback to ensure that jenkins is started and responding. After working with this for a while, I think it's easier to keep the relation and non-relation hooks handled separately, you don't need to re-install plugins when adding a slave for example.

« Back to merge proposal