juju-ci-tools

Merge lp://staging/~andrewjbeach/juju-ci-tools/bootstrap-args into lp://staging/juju-ci-tools

bootstrap-args
Merge into trunk

Proposed by Andrew James Beach on 2016-10-04

Status:	Merged
Merged at revision:	1631
Proposed branch:	lp://staging/~andrewjbeach/juju-ci-tools/bootstrap-args
Merge into:	lp://staging/juju-ci-tools
Diff against target:	124 lines (+58/-9) 2 files modified jujupy.py (+25/-9) tests/test_jujupy.py (+33/-0)
To merge this branch:	bzr merge lp://staging/~andrewjbeach/juju-ci-tools/bootstrap-args
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Aaron Bentley (community)		2016-10-04	Approve on 2016-10-04
Review via email: mp+307587@code.staging.launchpad.net

Description of the change

Added a new parameter to get_bootstrap_args: agent_version. It also appears
in the bootstrap methods so you can pass in the value.

This conficts with upload_tools, if both are given an exception is raised.

Modified get_bootstrap_args and bootstrap methods in EnvJujuClient.
Added tests for the new parameter in test_jujupy.py.

Revision history for this message

Aaron Bentley (abentley) wrote on 2016-10-04:

I don't see any handling for juju 1.x. Did you test it?

Since EnvJujuClient's bootstrap implementation is used by EnvJujuClient1X, but its get_bootstrap_args has not been updated, I expect to fail because too many parameters are supplied.

I would expect EnvJujuClient1X.get_bootstrap_args to:
1. always have a signature matching EnvJujuClient.get_bootstrap_args, so it would accept agent_version, defaulted to None (like EnvJujuClient2B2.get_bootstrap_args)
2. raise an exception (e.g. ValueError) if agent_version is not None.

review: Needs Information

lp://staging/~andrewjbeach/juju-ci-tools/bootstrap-args updated on 2016-10-04

1638. By Andrew James Beach on 2016-10-04: Added checks to make sure the versions that don't support the new bootstrap arguments don't get them.

Revision history for this message

Andrew James Beach (andrewjbeach) wrote on 2016-10-04:

EnvJujuClient1X doesn't use EnvJujuClient's bootstrap, it uses the one in from 2A2. I instead did that to 2B2, which does use EnvJujuClient's bootstrap. If you think it is important I could repeat the process on other versions.

Revision history for this message

Aaron Bentley (abentley) wrote on 2016-10-04:

Thanks for explaining. Sorry I missed the EnvJujuClient2A2 implementation. Ideally that would have the same signature as the EnvJujuClient implementation, but it seems to have diverged considerably without causing actual problems, so I guess we don't need to address that right now.

Aside from a couple of typos, this is good to land. I do have a couple of suggestions for the get_bootstrap_args logic, but they're not required.

review: Approve

lp://staging/~andrewjbeach/juju-ci-tools/bootstrap-args updated on 2016-10-04