Skip to content

{Compute} Update function of auto_shutdown_vm #17352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BigCat20196 merged 3 commits into from
Mar 24, 2021
Merged

{Compute} Update function of auto_shutdown_vm #17352

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3866605
Update function of auto_shutdown_vm
BigCat20196 Mar 18, 2021
dafb118
Update describe of auto_shurdown_vm()
BigCat20196 Mar 18, 2021
939db2c
Modify auto_shutdown_VM, so that users can use parameters to set wi…
BigCat20196 Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update describe of auto_shurdown_vm()
  • Loading branch information
@BigCat20196
BigCat20196 committed Mar 18, 2021
commit dafb118fd222f17412e554cf24d4bbc80ef380c2
10 changes: 5 additions & 5 deletions src/azure-cli/azure/cli/command_modules/vm/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,12 +1005,12 @@ def auto_shutdown_vm(cmd, resource_group_name, vm_name, off=None, email=None, we
notification_settings = None
if email:
Copy link
Member

@yungezz yungezz Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this service behavior? If no, this will be breaking change to end users: adding a required param for email.

Copy link
Contributor Author

@BigCat20196 BigCat20196 Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the webhook parameter is also required if the user needs to set --email. This issue is that the user doesn't know the settings -- email needs to be set -- webhook. I make this change to prompt the user that both parameters must exist at the same time for them to take effect

Copy link
Member

@qwordy qwordy Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow webhook and no email?

if not webhook:
raise CLIError('usage error: --webhook is a required parameter')
raise CLIError('usage error: --webhook is a required parameter when --email exists')
notification_settings = {
'emailRecipient': email,
'webhookUrl': webhook,
'timeInMinutes': 30,
'status': 'Enabled'
'emailRecipient': email,
'webhookUrl': webhook,
'timeInMinutes': 30,
'status': 'Enabled'
}
Copy link
Member

@qwordy qwordy Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

schedule = Schedule(status='Enabled',
target_resource_id=vm_id,
Expand Down