Merge lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args into lp:juju-ci-tools
- add-snap-to-default-juju-args
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 1918 |
Proposed branch: | lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args |
Merge into: | lp:juju-ci-tools |
Diff against target: |
292 lines (+49/-72) 7 files modified
assess_public_clouds.py (+4/-7) deploy_stack.py (+4/-27) tests/test_assess_bootstrap.py (+6/-3) tests/test_assess_public_clouds.py (+7/-8) tests/test_deploy_stack.py (+2/-2) tests/test_utility.py (+4/-12) utility.py (+22/-13) |
To merge this branch: | bzr merge lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Nicholas Skaggs (community) | Needs Information | ||
Review via email: mp+316779@code.launchpad.net |
Commit message
Description of the change
Tweak things to allow running with no arguments if you have the snap version of juju installed. Extra tests were added to clarify the order; GOPATH/bin/juju > /usr/bin/juju > /snap/bin/juju.
- 1876. By Nicholas Skaggs
-
simplify
- 1877. By Nicholas Skaggs
-
Verbose mode enabled for function comment
Nicholas Skaggs (nskaggs) wrote : | # |
Funny you bring up 'which juju', as indeed I wanted to modify this to use the same logic. The hardcoded paths feel silly when it's really just $PATH/$GOPATH + juju.
I don't remember why I stepped back from that approach, other than it must not have worked. It was the original implementation. hmmm.
Nicholas Skaggs (nskaggs) wrote : | # |
I attempted to reuse get_full_path for this, it can be nice, make it a utility function. However, it caused spiraling headaches all across the testsuite; into every single test. TestParseArgs everywhere needed a new mock to prevent the popen call. Given that, I think I'd prefer this.
Aaron Bentley (abentley) wrote : | # |
You don't need to do more, you need to do less. This change produces only 5 test failures.
=== modified file 'utility.py'
--- utility.py 2017-01-19 04:47:42 +0000
+++ utility.py 2017-02-13 20:19:51 +0000
@@ -338,7 +338,7 @@
- default=
+ default=None)
def add_basic_
Nicholas Skaggs (nskaggs) wrote : | # |
I consider the sane defaults useful and essential for letting other folks easily run tests. I think this represents the least invasive option for adding support for snapped juju, but I'm open to other ideas not yet explored. I don't think removing the ability to not require arguments is the right direction.
Aaron Bentley (abentley) wrote : | # |
Nothing in what I said would remove sane defaults. By setting default to None, our tests will default to whatever juju is in the $PATH, be that a snap or whatever is installed.
- 1878. By Nicholas Skaggs
-
swap get_full_path to get_juju_path in utility
- 1879. By Nicholas Skaggs
-
revert 1878
- 1880. By Nicholas Skaggs
-
wip
- 1881. By Nicholas Skaggs
-
wip
- 1882. By Nicholas Skaggs
-
fixed logging, fixed juju binary path
- 1883. By Nicholas Skaggs
-
lint
- 1884. By Nicholas Skaggs
-
merge trunk
Aaron Bentley (abentley) wrote : | # |
Thanks.
I'm a little unclear why you moved _generate_
I do think that the leading underscore is incorrect. This is a public function meant to be invoked outside utility.py, not a private function. But that's a quibble.
- 1885. By Nicholas Skaggs
-
make _generate_
default_ clean_dir public
Preview Diff
1 | === modified file 'assess_public_clouds.py' |
2 | --- assess_public_clouds.py 2017-02-22 16:08:27 +0000 |
3 | +++ assess_public_clouds.py 2017-03-01 20:36:24 +0000 |
4 | @@ -22,8 +22,9 @@ |
5 | ) |
6 | from utility import ( |
7 | _clean_dir, |
8 | + generate_default_clean_dir, |
9 | + add_arg_juju_bin, |
10 | configure_logging, |
11 | - _generate_default_binary, |
12 | LoggedException, |
13 | _to_deadline, |
14 | ) |
15 | @@ -101,10 +102,7 @@ |
16 | """Parse all arguments.""" |
17 | parser = ArgumentParser( |
18 | description='Assess basic quality of public clouds.') |
19 | - parser.add_argument('juju_bin', nargs='?', |
20 | - help='Full path to the Juju binary. By default, this' |
21 | - ' will use $GOPATH/bin/juju or /usr/bin/juju in that' |
22 | - ' order.', default=_generate_default_binary()) |
23 | + add_arg_juju_bin(parser) |
24 | parser.add_argument('logs', nargs='?', type=_clean_dir, |
25 | help='A directory in which to store logs. By default,' |
26 | ' this will use the current directory', default=None) |
27 | @@ -126,8 +124,7 @@ |
28 | |
29 | def default_log_dir(settings): |
30 | if settings.logs is None: |
31 | - settings.logs = BootstrapManager._generate_default_clean_dir( |
32 | - 'assess_public_clouds') |
33 | + settings.logs = generate_default_clean_dir('assess_public_clouds') |
34 | |
35 | |
36 | def main(): |
37 | |
38 | === modified file 'deploy_stack.py' |
39 | --- deploy_stack.py 2017-02-28 18:47:38 +0000 |
40 | +++ deploy_stack.py 2017-03-01 20:36:24 +0000 |
41 | @@ -9,12 +9,6 @@ |
42 | except ImportError: |
43 | from contextlib import ExitStack as nested |
44 | |
45 | - |
46 | -from datetime import ( |
47 | - datetime, |
48 | -) |
49 | - |
50 | -import errno |
51 | import glob |
52 | import logging |
53 | import os |
54 | @@ -63,6 +57,7 @@ |
55 | make_substrate_manager, |
56 | ) |
57 | from utility import ( |
58 | + generate_default_clean_dir, |
59 | add_basic_testing_arguments, |
60 | configure_logging, |
61 | ensure_deleted, |
62 | @@ -473,6 +468,8 @@ |
63 | args = deploy_job_parse_args() |
64 | configure_logging(args.verbose) |
65 | series = args.series |
66 | + if not args.logs: |
67 | + args.logs = generate_default_clean_dir(args.temp_env_name) |
68 | if series is None: |
69 | series = 'precise' |
70 | charm_series = series |
71 | @@ -706,29 +703,9 @@ |
72 | return self.controller_strategy.tear_down_client |
73 | |
74 | @classmethod |
75 | - def _generate_default_clean_dir(cls, temp_env_name): |
76 | - """Creates a new unique directory for logging and returns name""" |
77 | - logging.info('Environment {}'.format(temp_env_name)) |
78 | - test_name = temp_env_name.split('-')[0] |
79 | - timestamp = datetime.now().strftime("%Y%m%d%H%M%S") |
80 | - log_dir = os.path.join('/tmp', test_name, 'logs', timestamp) |
81 | - |
82 | - try: |
83 | - os.makedirs(log_dir) |
84 | - logging.info('Created logging directory {}'.format(log_dir)) |
85 | - except OSError as e: |
86 | - if e.errno == errno.EEXIST: |
87 | - logging.warn('"Directory {} already exists'.format(log_dir)) |
88 | - else: |
89 | - raise('Failed to create logging directory: {} ' + |
90 | - log_dir + |
91 | - '. Please specify empty folder or try again') |
92 | - return log_dir |
93 | - |
94 | - @classmethod |
95 | def from_args(cls, args): |
96 | if not args.logs: |
97 | - args.logs = cls._generate_default_clean_dir(args.temp_env_name) |
98 | + args.logs = generate_default_clean_dir(args.temp_env_name) |
99 | |
100 | # GZ 2016-08-11: Move this logic into client_from_config maybe? |
101 | if args.juju_bin == 'FAKE': |
102 | |
103 | === modified file 'tests/test_assess_bootstrap.py' |
104 | --- tests/test_assess_bootstrap.py 2017-02-28 12:55:09 +0000 |
105 | +++ tests/test_assess_bootstrap.py 2017-03-01 20:36:24 +0000 |
106 | @@ -219,7 +219,8 @@ |
107 | with assess_bootstrap_cxt(): |
108 | with self.sub_assess_mocks() as (base_mock, metadata_mock, |
109 | to_mock): |
110 | - assess_bootstrap(args) |
111 | + with patch('jujupy.ModelClient.get_full_path'): |
112 | + assess_bootstrap(args) |
113 | self.assertEqual(1, base_mock.call_count) |
114 | self.assertEqual(0, metadata_mock.call_count) |
115 | self.assertEqual(0, to_mock.call_count) |
116 | @@ -229,7 +230,8 @@ |
117 | with assess_bootstrap_cxt(): |
118 | with self.sub_assess_mocks() as (base_mock, metadata_mock, |
119 | to_mock): |
120 | - assess_bootstrap(args) |
121 | + with patch('jujupy.ModelClient.get_full_path'): |
122 | + assess_bootstrap(args) |
123 | self.assertEqual(0, base_mock.call_count) |
124 | self.assertEqual(1, metadata_mock.call_count) |
125 | self.assertEqual(0, to_mock.call_count) |
126 | @@ -239,7 +241,8 @@ |
127 | with assess_bootstrap_cxt(): |
128 | with self.sub_assess_mocks() as (base_mock, metadata_mock, |
129 | to_mock): |
130 | - assess_bootstrap(args) |
131 | + with patch('jujupy.ModelClient.get_full_path'): |
132 | + assess_bootstrap(args) |
133 | self.assertEqual(0, base_mock.call_count) |
134 | self.assertEqual(0, metadata_mock.call_count) |
135 | self.assertEqual(1, to_mock.call_count) |
136 | |
137 | === modified file 'tests/test_assess_public_clouds.py' |
138 | --- tests/test_assess_public_clouds.py 2017-02-10 20:39:24 +0000 |
139 | +++ tests/test_assess_public_clouds.py 2017-03-01 20:36:24 +0000 |
140 | @@ -46,9 +46,10 @@ |
141 | class TestParseArgs(TestCase): |
142 | |
143 | def test_parse_args(self): |
144 | - args = parse_args([]) |
145 | + with patch('utility.os.getenv', return_value=False): |
146 | + args = parse_args([]) |
147 | self.assertEqual(Namespace( |
148 | - deadline=None, debug=False, juju_bin='/usr/bin/juju', logs=None, |
149 | + deadline=None, debug=False, juju_bin=None, logs=None, |
150 | start=0, cloud_regions=None, |
151 | ), args) |
152 | |
153 | @@ -117,18 +118,16 @@ |
154 | |
155 | def test_default_log_dir(self): |
156 | settings = Namespace(logs=None) |
157 | - with patch( |
158 | - 'deploy_stack.BootstrapManager._generate_default_clean_dir', |
159 | - return_value='/tmp12345') as clean_dir_mock: |
160 | + with patch('assess_public_clouds.generate_default_clean_dir', |
161 | + return_value='/tmp12345') as clean_dir_mock: |
162 | default_log_dir(settings) |
163 | self.assertEqual('/tmp12345', settings.logs) |
164 | clean_dir_mock.assert_called_once_with(_LOCAL) |
165 | |
166 | def test_default_log_dir_provided(self): |
167 | settings = Namespace(logs='/tmpABCDE') |
168 | - with patch( |
169 | - 'deploy_stack.BootstrapManager._generate_default_clean_dir', |
170 | - autospec=True) as clean_dir_mock: |
171 | + with patch('assess_public_clouds.generate_default_clean_dir', |
172 | + autospec=True) as clean_dir_mock: |
173 | default_log_dir(settings) |
174 | self.assertEqual('/tmpABCDE', settings.logs) |
175 | self.assertFalse(clean_dir_mock.called) |
176 | |
177 | === modified file 'tests/test_deploy_stack.py' |
178 | --- tests/test_deploy_stack.py 2017-02-28 16:02:17 +0000 |
179 | +++ tests/test_deploy_stack.py 2017-03-01 20:36:24 +0000 |
180 | @@ -1149,7 +1149,7 @@ |
181 | |
182 | def test_deploy_job_changes_series_with_win(self): |
183 | args = Namespace( |
184 | - series='windows', temp_env_name=None, env=None, upgrade=None, |
185 | + series='windows', temp_env_name='windows', env=None, upgrade=None, |
186 | charm_prefix=None, bootstrap_host=None, machine=None, logs=None, |
187 | debug=None, juju_bin=None, agent_url=None, agent_stream=None, |
188 | keep_env=None, upload_tools=None, with_chaos=None, jes=None, |
189 | @@ -1162,7 +1162,7 @@ |
190 | |
191 | def test_deploy_job_changes_series_with_centos(self): |
192 | args = Namespace( |
193 | - series='centos', temp_env_name=None, env=None, upgrade=None, |
194 | + series='centos', temp_env_name='centos', env=None, upgrade=None, |
195 | charm_prefix=None, bootstrap_host=None, machine=None, logs=None, |
196 | debug=None, juju_bin=None, agent_url=None, agent_stream=None, |
197 | keep_env=None, upload_tools=None, with_chaos=None, jes=None, |
198 | |
199 | === modified file 'tests/test_utility.py' |
200 | --- tests/test_utility.py 2017-02-25 06:09:40 +0000 |
201 | +++ tests/test_utility.py 2017-03-01 20:36:24 +0000 |
202 | @@ -276,11 +276,11 @@ |
203 | |
204 | def test_no_args(self): |
205 | cmd_line = [] |
206 | - parser = add_basic_testing_arguments(ArgumentParser(), deadline=True) |
207 | - with patch('utility.os.getenv', return_value=''): |
208 | - args = parser.parse_args(cmd_line) |
209 | + parser = add_basic_testing_arguments(ArgumentParser(), |
210 | + deadline=True) |
211 | + args = parser.parse_args(cmd_line) |
212 | self.assertEqual(args.env, 'lxd') |
213 | - self.assertEqual(args.juju_bin, '/usr/bin/juju') |
214 | + self.assertEqual(args.juju_bin, None) |
215 | |
216 | self.assertEqual(args.logs, None) |
217 | |
218 | @@ -292,14 +292,6 @@ |
219 | self.assertEqual(temp_env_name_arg[2:4], ['temp', 'env']) |
220 | self.assertIs(None, args.deadline) |
221 | |
222 | - def test_default_binary(self): |
223 | - cmd_line = [] |
224 | - with patch('utility.os.getenv', return_value='/tmp'): |
225 | - with patch('utility.os.path.isfile', return_value=True): |
226 | - parser = add_basic_testing_arguments(ArgumentParser()) |
227 | - args = parser.parse_args(cmd_line) |
228 | - self.assertEqual(args.juju_bin, '/tmp/bin/juju') |
229 | - |
230 | def test_positional_args(self): |
231 | cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest'] |
232 | parser = add_basic_testing_arguments(ArgumentParser(), deadline=True) |
233 | |
234 | === modified file 'utility.py' |
235 | --- utility.py 2017-02-25 06:09:40 +0000 |
236 | +++ utility.py 2017-03-01 20:36:24 +0000 |
237 | @@ -208,6 +208,26 @@ |
238 | return 'unknown_test' |
239 | |
240 | |
241 | +def generate_default_clean_dir(temp_env_name): |
242 | + """Creates a new unique directory for logging and returns name""" |
243 | + logging.debug('Environment {}'.format(temp_env_name)) |
244 | + test_name = temp_env_name.split('-')[0] |
245 | + timestamp = datetime.now().strftime("%Y%m%d%H%M%S") |
246 | + log_dir = os.path.join('/tmp', test_name, 'logs', timestamp) |
247 | + |
248 | + try: |
249 | + os.makedirs(log_dir) |
250 | + logging.info('Created logging directory {}'.format(log_dir)) |
251 | + except OSError as e: |
252 | + if e.errno == errno.EEXIST: |
253 | + logging.warn('"Directory {} already exists'.format(log_dir)) |
254 | + else: |
255 | + raise('Failed to create logging directory: {} ' + |
256 | + log_dir + |
257 | + '. Please specify empty folder or try again') |
258 | + return log_dir |
259 | + |
260 | + |
261 | def _generate_default_temp_env_name(): |
262 | """Creates a new unique name for environment and returns the name""" |
263 | # we need to sanitize the name |
264 | @@ -216,16 +236,6 @@ |
265 | return '{}-{}-temp-env'.format(test_name, timestamp) |
266 | |
267 | |
268 | -def _generate_default_binary(): |
269 | - """Returns GOPATH juju binary if it exists, otherwise /usr/bin/juju""" |
270 | - if os.getenv('GOPATH'): |
271 | - go_bin = os.getenv('GOPATH') + '/bin/juju' |
272 | - if os.path.isfile(go_bin): |
273 | - return go_bin |
274 | - |
275 | - return '/usr/bin/juju' |
276 | - |
277 | - |
278 | def _to_deadline(timeout): |
279 | return datetime.utcnow() + timedelta(seconds=int(timeout)) |
280 | |
281 | @@ -233,9 +243,8 @@ |
282 | def add_arg_juju_bin(parser): |
283 | parser.add_argument('juju_bin', nargs='?', |
284 | help='Full path to the Juju binary. By default, this' |
285 | - ' will use $GOPATH/bin/juju or /usr/bin/juju in that' |
286 | - ' order.', |
287 | - default=_generate_default_binary()) |
288 | + ' will use $PATH/juju', |
289 | + default=None) |
290 | |
291 | |
292 | def add_basic_testing_arguments(parser, using_jes=False, deadline=True, |
I don't think this is right. If there is a default juju, it should be the juju returned by "which juju", i.e. the one in your PATH. If you have the snap installed, the snap juju should be in your PATH.
client_from_config already gives you this value if juju_path is None.