[Compute] vmss create/update: add --terminate-notification and --terminate-notification-time parameters to support terminate scheduled event configurability. Issue #10124 by qwordy · Pull Request #10391 · Azure/azure-cli

qwordy · 2019-09-02T02:20:24Z

[VM] vmss create: --terminate-notification, add terminate scheduled event configurability #10124

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

yugangw-msft

Great start, Feiyue! I left a few minor comments. Please also author an example and verify it

mmyyrroonn

Hi. Just some comments

Now, only the create command support this argument. However, the related issue also ask for update command. Please add this argument into update command.
I cannot understand, how can the customer disable this field after creating it.
At least, please add a test command for this argument.
You can include "Fix #10124", "Fixes #10124" or "Fixed #10124" in your first comment. When this PR is merged, the related issue will be closed automatically.

yugangw-msft · 2019-09-02T03:17:48Z

I cannot understand, how can the customer disable this field after creating it.

Good point. I believe "az vmss update" should handle it through the generic updater, but let us cross check.

qwordy · 2019-09-02T05:58:25Z

Hi. Just some comments

* Now, only the create command support this argument. However, the related issue also ask for update command. Please add this argument into update command.

* I cannot understand, how can the customer disable this field after creating it.

* At least, please add a test command for this argument.

* You can include "Fix #10124", "Fixes #10124" or "Fixed #10124" in your first comment. When this PR is merged, the related issue will be closed automatically.

Good idea.
update --set command. But it's better to provide a dedicated parameter.
OK
OK
Thank you for your review.

mmyyrroonn · 2019-09-04T05:10:50Z


+    for scope in ['vmss create', 'vmss update']:
+        with self.argument_context(scope) as c:
+            c.argument('terminate_notification', min_api='2019-03-01', arg_type=get_three_state_flag(),


Usually we use enable_terminate_notification

https://docs.microsoft.com/en-us/cli/azure/vm?view=azure-cli-latest#az-vm-create

vm create has 3 boolean parameters now. They don't have a enable_ prefix.

enable_ means you can only enable it. You can not pass true or false.

Abbreviation is not urgent and is not pervasive now. If this parameter is used heavily, we can add it.

three_flags_arguments supports user just inputs --terminate-notification without any value followed. So it's quite confusing to use this argument like that.

I followed the pattern here.

azure-cli/src/azure-cli/azure/cli/command_modules/vm/_params.py

Line 495 in 844005a

c.argument('accelerated_networking', arg_type=get_three_state_flag(),

https://docs.microsoft.com/en-us/cli/azure/vmss?view=azure-cli-latest#az-vmss-create

It's time to escalate.
@yugangw-msft What's the best practice?

Just in suggestion, for storage module, we also use enable_xxx for such argument to enable a flag or argument.

There is one parameter called "--ultra-ssd-enabled" in below doc:
https://docs.microsoft.com/en-us/cli/azure/vmss?view=azure-cli-latest#az-vmss-create
Maybe you can also follow that pattern and use "terminate-notification-enabled" so that these two parameters can share the same prefix "terminate-notification".
@yugangw-msft can make the call for name convention.

let us go with enable_terminate_notification. accelerated_networking is a questionable naming which I probably should not have named that way.
(EDIT) also, let us use this opportunity to unify the naming decision on such arguments moving forward. I cross checked a few commands, right now we are not consistent, i am seeing enable_xxx, xxx_enabled, or just xxx

Thank you for your reviews.

Juliehzl · 2019-09-04T11:34:22Z

    if secrets:
        secrets = _merge_secrets([validate_file_or_dict(secret) for secret in secrets])

+    validate_terminate_notification(terminate_notification, terminate_notification_time)


This validate function is used only for arguments, right? If yes, it should be executed before command handler and you should specify your argument validator in argument context like https://github.com/Azure/azure-cli/blob/dev/src/azure-cli/azure/cli/command_modules/vm/_params.py#L238.

It seems reasonable.

Juliehzl · 2019-09-04T11:40:16Z

 azure-mgmt-cdn==3.1.0
 azure-mgmt-cognitiveservices==5.0.0
-azure-mgmt-compute==6.0.0
+azure-mgmt-compute==7.0.0


Can you declare package upgrade in changelog?

Can you declare package upgrade in changelog?

OK

yugangw-msft · 2019-09-04T13:40:35Z


+**Compute**
+
+* vmss create/update: Add --terminate-notification and --terminate-notification-time parameters to support terminate scheduled event configurability.


For create command, we probably only need --terminate-notification-time. As long as the time argument value is provided, CLI should enable the notification. I assume by default the notification is off, right? For update, we can expose both. The context is the create command has got too many fields, hence I recommend we use a bit more defaults.

Yes. It is turned off by default. OK.

qwordy requested review from mmyyrroonn and yugangw-msft September 2, 2019 02:20

qwordy self-assigned this Sep 2, 2019

yugangw-msft reviewed Sep 2, 2019

View reviewed changes

Comment thread src/azure-cli/azure/cli/command_modules/vm/_params.py Outdated

mmyyrroonn suggested changes Sep 2, 2019

View reviewed changes

yugangw-msft reviewed Sep 2, 2019

View reviewed changes

Comment thread src/azure-cli/HISTORY.rst Outdated

qwordy requested a review from Juliehzl September 3, 2019 07:13

mmyyrroonn reviewed Sep 4, 2019

View reviewed changes

qwordy requested review from yonzhan and zikalino September 4, 2019 06:25

Juliehzl reviewed Sep 4, 2019

View reviewed changes

yugangw-msft reviewed Sep 4, 2019

View reviewed changes

qwordy closed this Sep 6, 2019

qwordy force-pushed the dev branch from e74c034 to 2cac7fb Compare September 6, 2019 07:23

qwordy mentioned this pull request Sep 6, 2019

[Compute] vmss create/update: add --enable-terminate-notification (only for update) and --terminate-notification-time parameters to support terminate scheduled event configurability. Issue #10124 #10445

Merged

2 tasks


		Compute

		* vmss create/update: Add --terminate-notification and --terminate-notification-time parameters to support terminate scheduled event configurability.

Conversation

qwordy commented Sep 2, 2019 • edited by mmyyrroonn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmyyrroonn left a comment

Choose a reason for hiding this comment

Uh oh!

yugangw-msft commented Sep 2, 2019

Uh oh!

Uh oh!

qwordy commented Sep 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwordy Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qwordy commented Sep 2, 2019 •

edited by mmyyrroonn

Loading

qwordy Sep 4, 2019 •

edited

Loading

yugangw-msft Sep 4, 2019 •

edited

Loading

yugangw-msft Sep 4, 2019 •

edited

Loading