Merge lp:~daker/pyjuju/small-fix into lp:pyjuju

Proposed by Adnane Belmadiaf
Status: Work in progress
Proposed branch: lp:~daker/pyjuju/small-fix
Merge into: lp:pyjuju
Diff against target: 78 lines (+45/-9)
2 files modified
ensemble/control/shutdown.py (+13/-9)
ensemble/tests/test_shutdown.py (+32/-0)
To merge this branch: bzr merge lp:~daker/pyjuju/small-fix
Reviewer Review Type Date Requested Status
Kapil Thangavelu (community) Approve
William Reade (community) Approve
Review via email: mp+68038@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ahmed Kamal (kim0) wrote :

I would actually love it if shutdown took a -y argument as well

lp:~daker/pyjuju/small-fix updated
277. By Adnane Belmadiaf

Added -y argument to shutdown command.

Revision history for this message
William Reade (fwereade) wrote :

Line 13 goes well over 80 characters, please break it.

Also: can I suggest that you use a name other than "choice" for the dest and variable names? I think something like "confirmed" would be a bit clearer (which environment you're using is also a choice, and so will be any other options we add in the future).

Otherwise, it works perfectly :).

review: Needs Resubmitting
lp:~daker/pyjuju/small-fix updated
278. By Adnane Belmadiaf

* Added --yes argument to shutdown command
* Changed dest variable name
* Other fixes

Revision history for this message
William Reade (fwereade) wrote :

Sorry, I think I was unclear before: I meant that the source line should be broken, not that the help text itself needed a line break.

I also just noticed that you've put spaces before colons, which isn't necessary. Please remove those, but otherwise I'm happy; +1.

review: Approve
lp:~daker/pyjuju/small-fix updated
279. By Adnane Belmadiaf

* Small fix

280. By Adnane Belmadiaf

* Remove spaces

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

There aren't any tests here. There should be a default=False for the param, especially given the is False condition afaics.

review: Needs Fixing
lp:~daker/pyjuju/small-fix updated
281. By Adnane Belmadiaf

* Added a default value

282. By Adnane Belmadiaf

* Added test case, props hazmat

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Looks good +1

review: Approve
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

one additional question.. have you signed the contributor agreement per?
http://www.canonical.com/contributors

else it looks good to get merged.

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

marking work in progress while waiting on contrib agreement.

Revision history for this message
Adnane Belmadiaf (daker) wrote :

Ok, sorry for the late reply, i signed the agreement.

Unmerged revisions

282. By Adnane Belmadiaf

* Added test case, props hazmat

281. By Adnane Belmadiaf

* Added a default value

280. By Adnane Belmadiaf

* Remove spaces

279. By Adnane Belmadiaf

* Small fix

278. By Adnane Belmadiaf

* Added --yes argument to shutdown command
* Changed dest variable name
* Other fixes

277. By Adnane Belmadiaf

Added -y argument to shutdown command.

276. By Adnane Belmadiaf

Small fix.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'ensemble/control/shutdown.py'
--- ensemble/control/shutdown.py 2011-06-09 15:13:03 +0000
+++ ensemble/control/shutdown.py 2011-07-21 17:57:49 +0000
@@ -9,9 +9,12 @@
9 sub_parser.add_argument(9 sub_parser.add_argument(
10 "--environment", "-e",10 "--environment", "-e",
11 help="Ensemble environment to operate in.")11 help="Ensemble environment to operate in.")
12 sub_parser.add_argument(
13 "--yes", "-y", dest='confirmed', action='store_true', default=False,
14 help="Destroy all machines and services in the environment "
15 "automatically.")
12 return sub_parser16 return sub_parser
1317
14
15@inlineCallbacks18@inlineCallbacks
16def command(options):19def command(options):
17 """20 """
@@ -19,14 +22,15 @@
19 """22 """
20 environment = get_environment(options)23 environment = get_environment(options)
21 provider = environment.get_machine_provider()24 provider = environment.get_machine_provider()
2225 confirmed = options.confirmed
23 value = raw_input(26 if confirmed is False:
24 "Warning, this will destroy all machines and services in the\n"27 value = raw_input(
25 "environment. Continue [y/N]")28 "Warning, this will destroy all machines and services in the\n"
2629 "environment. Continue [y/N]: ")
27 if value.strip().lower() not in ("y", "yes"):30
28 options.log.info("Shutdown aborted")31 if value.strip().lower() not in ("y", "ye", "yes"):
29 returnValue(None)32 options.log.info("Shutdown aborted")
33 returnValue(None)
30 options.log.info("Shutting down environment %r (type: %s)..." % (34 options.log.info("Shutting down environment %r (type: %s)..." % (
31 environment.name, environment.type))35 environment.name, environment.type))
32 yield provider.shutdown()36 yield provider.shutdown()
3337
=== added file 'ensemble/tests/test_shutdown.py'
--- ensemble/tests/test_shutdown.py 1970-01-01 00:00:00 +0000
+++ ensemble/tests/test_shutdown.py 2011-07-21 17:57:49 +0000
@@ -0,0 +1,32 @@
1@inlineCallbacks
2def test_shutdown_with_cli_confirm(self):
3 """
4 'ensemble-control shutdown' will shutdown all configured instances
5 in the specified environment. The confirmation prompt can be bypassed
6 via a cli option.
7 """
8 config = {
9 "ensemble": "environments",
10 "environments": {"firstenv": {"type": "dummy"},
11 "secondenv": {"type": "dummy"}}}
12 self.write_config(dump(config))
13 finished = self.setup_cli_reactor()
14
15 envs = set(("firstenv", "secondenv"))
16
17 def track_shutdown_call(self):
18 envs.remove(self.environment_name)
19 return succeed(True)
20
21 provider = self.mocker.patch(MachineProvider)
22 provider.shutdown()
23 self.mocker.call(track_shutdown_call, with_object=True)
24
25 self.setup_exit(0)
26 self.mocker.replay()
27
28 main(["shutdown", "-y", "-e", "firstenv"])
29 yield finished
30 msg = "Shutting down environment 'firstenv' (type: dummy)..."
31 self.assertIn(msg, self.log.getvalue())
32 self.assertEqual(envs, set(["secondenv"]))

Subscribers

People subscribed via source and target branches

to status/vote changes: