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
Next Next commit
Update function of auto_shutdown_vm
  • Loading branch information
@BigCat20196
BigCat20196 committed Mar 18, 2021
commit 3866605d1273d564114dbce50f205361c5fcb5bc
14 changes: 8 additions & 6 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 @@ -1003,13 +1003,15 @@ def auto_shutdown_vm(cmd, resource_group_name, vm_name, off=None, email=None, we
raise CLIError('usage error: --time is a required parameter')
daily_recurrence = {'time': time}
notification_settings = None
if webhook:
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')
Copy link
Member

@qwordy qwordy Mar 18, 2021

Choose a reason for hiding this comment

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

When is it a required parameter? The error message could be better.

notification_settings = {
'emailRecipient': email,
'webhookUrl': webhook,
'timeInMinutes': 30,
'status': 'Enabled'
}
'emailRecipient': email,
'webhookUrl': webhook,
'timeInMinutes': 30,
'status': 'Enabled'
}
Copy link
Member

@qwordy qwordy Mar 18, 2021

Choose a reason for hiding this comment

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

Why add indent?

Copy link
Contributor Author

@BigCat20196 BigCat20196 Mar 18, 2021

Choose a reason for hiding this comment

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

Yeah, it's unnecessary. I took it out

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,
daily_recurrence=daily_recurrence,
Expand Down