Support Wait/No-Wait by yugangw-msft · Pull Request #1463 · Azure/azure-cli

yugangw-msft · 2016-11-29T22:21:26Z

Submit it to leverage CI to catch regressions. Still finalizing the change, but feel free to put in comments.
Fix #1071, Fix #1169

This change enable author to expose --no-wait from long running command, and also expose wait commands to wait on any conditions based on read api.

Command
    az vm wait

Arguments

Resource Id Arguments
    --ids              : One or more resource IDs. If provided, no other 'Resource Id' arguments
                         should be specified.
    --name -n          : The name of the virtual machine.
    --resource-group -g: Name of resource group.

Wait Condition Arguments
    --created          : Wait till created with 'provisioningState' at 'Succeeded'.
    --custom           : Wait until the condition satisfies a custom JMESPath query. E.g.
                         provisioningState!='InProgress',
                         instanceView.statuses[?code=='PowerState/running'].
    --deleted          : Wait till deleted.
    --exists           : Wait till the resource exists.
    --interval         : Polling interval in seconds.  Default: 30.
    --timeout          : Maximum wait in seconds.  Default: 3600.
    --updated          : Wait till updated with provisioningState at 'Succeeded'.

mention-bot · 2016-11-29T22:21:29Z

@yugangw-msft, thanks for your PR! By analyzing the history of the files in this pull request, we identified @derekbekoe, @johanste, @tjprescott, @brendandburns and @zooba to be potential reviewers.

johanste · 2016-11-29T22:48:44Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

@@ -240,10 +241,10 @@ def register_extra_cli_argument(command, dest, **kwargs):
    _cli_extra_argument_registry[command][dest] = CliCommandArgument(dest, **kwargs)

 def cli_command(module_name, name, operation,


Should we add a **kwargs argument if we expect to add more of these "pass-through" parameters?

Agreed. After the main logic gets reviewed, I will consolidate

Just so long as the public facing cli_command(...) call does not use kwargs (coughresource_idcough)

tjprescott

Mostly just some comments from what I saw in the help text. I'll do a more detailed review when you are ready to merge it.

tjprescott · 2016-11-29T22:45:24Z

src/azure-cli-core/azure/cli/core/commands/arm.py

+    cmd.add_argument('deleted', '--deleted', action='store_true', arg_group=group_name,
+                     help='wait till deleted')
+    cmd.add_argument('created', '--created', action='store_true', arg_group=group_name,
+                     help="wait till created with 'provisitioningState' at 'Succeeded'")


typo: provisioningState instead of provisitioningState. Also line 410.

tjprescott · 2016-11-29T22:47:27Z

src/azure-cli-core/azure/cli/core/commands/arm.py

+                     help="wait till updated with provisitioningState at 'Succeeded'")
+    cmd.add_argument('exists', '--exists', action='store_true', arg_group=group_name,
+                     help="wait till the resource exists")
+    cmd.add_argument('property', '--property', arg_group=group_name,


Recommend '--custom' and a message like "Wait until the condition satisfies a custom JMESPath query."

tjprescott · 2016-11-29T22:50:04Z

src/azure-cli-core/azure/cli/core/commands/arm.py

+
+    cmd = CliCommand(name, handler, table_transformer=table_transformer,
+                     arguments_loader=arguments_loader)
+    group_name = 'Wait'


Recommend 'Wait Condition'

colemickens · 2016-11-30T00:06:34Z

What would example usage of this look like with a template deployment for example? I guess I'd name the deployment and then I could later wait on the deployment?

az resource group create --name coletest1 --location westus
az resource group deployment create \
    --name deploy01 \
    --template-file "./template.json" \
    --parameters "@./parameters.json" \
    --nowait
az resource group deployment wait --name deploy01

Something like that roughly?

tjprescott · 2016-11-30T00:09:37Z

@colemickens that's right. Each resource (deployment being the resource in this case) will expose a wait command which lets you poll on some property that is exposed through that resource's GET method. You would probably have to add the --created flag to your wait though.

yugangw-msft · 2016-11-30T00:09:59Z

@colemickens, that is correct.

    az resource group deployment wait --name deploy01 --created

yugangw-msft · 2016-11-30T00:11:42Z

@colemickens, you can also use --custom to wait on any other conditions.

colemickens · 2016-11-30T00:14:08Z

In this case --created seems like a bit of a misnomer. Why --created and not --success or --succeeded ?

In this case, and normal ARM teminology... the deployment is "created" as soon as the 201/202 is sent back... and transitions from InProgess to Succeeded.

It's a tiny bit confusing, but I guess for other resources the meaning is pretty clear.

derekbekoe · 2016-11-30T00:05:18Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

So if 'raw' not in kwargs, we essentially ignore the --no-wait parameter?
Will this be a common thing, if so, consider adding logger.warning message like 'Ignoring no wait because ...'.

The no-wait is a parameter alias on raw. This if condition makes sure the command returns nothing on no-wait, since we don't have yet the final result to return to users.

Or perhaps it should be an error instead of a warning. If you register no-wait on a command that doesn't support raw, then I would argue it would be better to have that fail positively than just vaguely "not work".

I will raise exception in _introspection.py where we do parameter excursion and alias

derekbekoe · 2016-11-30T00:10:02Z

src/azure-cli-core/azure/cli/core/commands/arm.py

I think to match the parameter names:
create -> created
delete -> deleted
exits -> exists

If 'property' a valid flag?

thanks, I forgot to rename it to custom

Looks like 'property' was renamed 'custom' so this just needs to be updated. Also, 'exists' is misspelled 'exits'.

We might consider having a default here instead of raising an error when nothing is specified.

Also, recommend this form for the error: "incorrect usage: --created | --updated | --deleted | --exists | --custom JMESPATH"

derekbekoe · 2016-11-30T00:11:16Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Do the wait commands return objects?
Looks like it won't return anything?

Correct. When the wait is successful you are returned to the command prompt. GET is an implementation detail behind the wait, but there's no need to return the object from the final GET.

derekbekoe · 2016-11-30T00:11:56Z

src/azure-cli-core/azure/cli/core/commands/arm.py

More descriptive message like 'Wait operation timed-out after X seconds'

derekbekoe · 2016-11-30T00:14:31Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Would be good if there was a validator on this parameter to check that the string is a valid JMESPath query so the command will fail even before any API calls are made.

I think it does that at line 423 essentially. It compiles the JMESPath, which should raise an error if it is invalid.

Yep. Invalid JmesPath will be raised

yugangw-msft · 2016-11-30T00:22:54Z

@colemickens

In this case --created seems like a bit of a misnomer. Why --created and not --success or --succeeded ?

I felt about the same. Let me think about whether I can improve the naming. For lots of "create" commands, --created does make sense though.

tjprescott · 2016-11-30T00:23:12Z

@colemickens --created because your command was to create the deployment, but I see your point. I think --exists most closely matches what you are describing. The option names are still in flux so feedback is good. :)

But I don't think customers will care so much what the ARM team considers created, so we want to go with what will make sense to them.

tjprescott

Overall, I'm really excited to get this change in. Changes requested are mostly typos, but also some other questions.

tjprescott · 2016-11-30T00:30:03Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

@@ -240,10 +241,10 @@ def register_extra_cli_argument(command, dest, **kwargs):
    _cli_extra_argument_registry[command][dest] = CliCommandArgument(dest, **kwargs)

 def cli_command(module_name, name, operation,


Just so long as the public facing cli_command(...) call does not use kwargs (coughresource_idcough)

tjprescott · 2016-11-30T00:32:00Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

Or perhaps it should be an error instead of a warning. If you register no-wait on a command that doesn't support raw, then I would argue it would be better to have that fail positively than just vaguely "not work".

tjprescott · 2016-11-30T00:40:19Z

src/azure-cli-core/azure/cli/core/commands/arm.py

This should be positively resolved during the command registration, so I would argue you simply get the raw property here.

My concern is it is possible (though very unlikely) an custom method might contain raw parameter for other purpose.

Or we can consider to letting the command registration flow to expose one more argument say no_wait_param, and default to raw. This way we have the decoupling and be explicit, of course the registration logic (public facing API) looks a bit more complex.

I made the change. Please take a look. It is easy to rollback if we feel the complexity not worth it

tjprescott · 2016-11-30T00:42:20Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Since wait always returns nothing, table_transformer will never apply.

tjprescott · 2016-11-30T00:43:58Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Would this be a TypeError? Also, recommend the message contain type(getter_op) instead of just getter_op. If someone passed 10 it would say "Got '10'" instead of "Got 'int'".

Okay, updated. Note, this is the code pattern which exists at several places. I just reused it.

tjprescott · 2016-11-30T00:47:17Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Looks like 'property' was renamed 'custom' so this just needs to be updated. Also, 'exists' is misspelled 'exits'.

We might consider having a default here instead of raising an error when nothing is specified.

Also, recommend this form for the error: "incorrect usage: --created | --updated | --deleted | --exists | --custom JMESPATH"

tjprescott · 2016-11-30T00:48:21Z

src/azure-cli-core/azure/cli/core/commands/arm.py

Correct. When the wait is successful you are returned to the command prompt. GET is an implementation detail behind the wait, but there's no need to return the object from the final GET.

tjprescott · 2016-11-30T00:49:39Z

src/azure-cli-core/azure/cli/core/commands/arm.py

I think it does that at line 423 essentially. It compiles the JMESPath, which should raise an error if it is invalid.

yugangw-msft · 2016-11-30T05:57:05Z

@tjprescott, please take another look.

tjprescott · 2016-11-30T16:55:02Z

src/azure-cli-core/azure/cli/core/commands/_introspection.py

    if not expose_raw_as_no_wait:
        excluded_params.append('raw')
+    elif not 'raw' in args:
+        raise ValueError("There is no 'raw' argument exposed for no-wait")


This would confuse me if I were not familiar with the implementation of no-wait. I would suggest something like "Unable to enable no-wait option. Operation '{}' does not have a 'raw' parameter.".format(operation.name)

In other words, make it clear that the method must have the 'raw' capability in order to expose no-wait through this argument.

tjprescott · 2016-11-30T17:00:04Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

 def cli_command(module_name, name, operation,
-                client_factory=None, transform=None, table_transformer=None, expose_no_wait=False):
+                client_factory=None, transform=None, table_transformer=None,
+                expose_no_wait=False, no_wait_param=None):


Having both expose_no_wait and no_wait_param seems redundant. If no_wait_param is None then you implicitly haven't enabled it. If it is anything else, then you have enabled it. However, I do like this way, of simply specifying the parameter name as it accomplishes the same thing with a little more flexibility.

I would add a check here to verify that the param no_wait_param refers to is a boolean type and throw if it is not. Otherwise someone will try and use a string field or something and things will work in mysterious ways.

tjprescott

I like the decoupling. Adds a little for flexibility for not much more complexity. Added a couple more comments for some minor changes.

tjprescott

LGTM!

yugangw-msft added 4 commits November 28, 2016 14:51

Handle wait/no-wait

8803466

expose wait till 'exists'

7b9983a

add group no_wait test

ec85edb

add test for app-gateway and vm

3f12e83

azurecla added the cla-not-required label Nov 29, 2016

johanste reviewed Nov 29, 2016

View reviewed changes

tjprescott reviewed Nov 29, 2016

View reviewed changes

tjprescott mentioned this pull request Nov 30, 2016

az resource group delete should accept a --fast or --nowait flag #1071

Closed

derekbekoe reviewed Nov 30, 2016

View reviewed changes

yugangw-msft force-pushed the nowait3 branch from 82b156e to acaa1fa Compare November 30, 2016 00:28

address CI failure and quick review feedback

edf62f4

yugangw-msft force-pushed the nowait3 branch from acaa1fa to edf62f4 Compare November 30, 2016 00:35

tjprescott suggested changes Nov 30, 2016

View reviewed changes

yugangw-msft added 2 commits November 29, 2016 17:31

address new review feedback

653056f

decouple 'raw' from 'no-wait'

f56d50d

yugangw-msft changed the title ~~[do not merge]: Wait/No-Wait~~ Support Wait/No-Wait Nov 30, 2016

report correct error if no wait option is specified

c2dd5ff

safely remove no_wait_param

6011f5c

tjprescott reviewed Nov 30, 2016

View reviewed changes

tjprescott suggested changes Nov 30, 2016

View reviewed changes

remove expose_no_wait as no_wait_param is enough to control the logic

bfefff3

tjprescott approved these changes Nov 30, 2016

View reviewed changes

yugangw-msft force-pushed the nowait3 branch from 51c0786 to 63dd5ec Compare November 30, 2016 22:12

onboard a few custom command to have no-wait

e6bf2c4

yugangw-msft force-pushed the nowait3 branch from 63dd5ec to e6bf2c4 Compare November 30, 2016 22:46

yugangw-msft merged commit b60590d into Azure:master Dec 1, 2016

tjprescott mentioned this pull request Dec 12, 2016

Add wait/no-wait support to Commands #1528

Open

23 tasks

yugangw-msft deleted the nowait3 branch December 27, 2016 03:52

haroldrandom added the cla-not-required label Oct 25, 2019

00Kai0 pushed a commit to 00Kai0/azure-cli that referenced this pull request Apr 7, 2021

[Blueprint] Use experimental tag, update doc (Azure#1463)

d529b6f

		@@ -240,10 +241,10 @@ def register_extra_cli_argument(command, dest, **kwargs):
		_cli_extra_argument_registry[command][dest] = CliCommandArgument(dest, **kwargs)

		def cli_command(module_name, name, operation,

Conversation

yugangw-msft commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colemickens commented Nov 30, 2016

Uh oh!

tjprescott commented Nov 30, 2016

Uh oh!

yugangw-msft commented Nov 30, 2016

Uh oh!

yugangw-msft commented Nov 30, 2016

Uh oh!

colemickens commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjprescott commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft commented Nov 29, 2016 •

edited

Loading

yugangw-msft commented Nov 30, 2016 •

edited

Loading

tjprescott commented Nov 30, 2016 •

edited

Loading