-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Add wait commands and --no-wait support #2524
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2524 +/- ##
==========================================
- Coverage 72.09% 72.02% -0.07%
==========================================
Files 362 362
Lines 19758 19764 +6
Branches 2909 2912 +3
==========================================
- Hits 14244 14236 -8
- Misses 4600 4610 +10
- Partials 914 918 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one question on 'generic update' logic, the rest LGTM
ordered_arguments = args.pop('ordered_arguments', []) | ||
for item in ['properties_to_add', 'properties_to_set', 'properties_to_remove']: | ||
if args[item]: | ||
raise CLIError("Unexpected '{}' was not empty.".format(item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we just say '{}' was not expected'
. I don't know much about generic update though, but in case it helps
Also why we want to error out here? I assume we will apply their values on sending out the update
payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an Action somewhere that removes the entries in these lists and puts them into the "ordered_args" argument. So if anything was left in these, I want to raise an error so I know there's a bug in the generic update logic before I delete them.
|
||
try: | ||
client = factory() if factory else None | ||
except TypeError: | ||
client = factory(None) if factory else None | ||
|
||
getterargs = {key: val for key, val in args.items() | ||
if key in get_arguments_loader()} | ||
getterargs = {key: val for key, val in args.items() if key in get_arguments_loader()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the cost of get_arguments_loader()
? Can we call it once and save the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only calling it once in the handler. This method doesn't get the "argument_loader" in the sense that "get_foo" would get the foo. It returns the CLI Arguments for the "get" side of the update. There's a corresponding "set_arguments_loader" that returns the CLI Arguments for the "set" side of the update. That's also only called once in the handler.
* Azure/master: (478 commits) vm live test: allow more valid power states on vmss test verifications (Azure#2564) rbac:catch more graph error (Azure#2567) appservice: support to create plan when create a webapp (Azure#2550) Update storage tests (Azure#2556) Change PEP8 check filter from whitelist to blacklist (Azure#2557) Add scenario tests documentation (Azure#2555) [ACS] Adding support for configuring a default ACS cluster (Azure#2554) [ACS] Provide a short name alias for the orchestrator type flag (Azure#2553) Sql Import/Export CLI commands and test (Azure#2538) Fix format bug. (Azure#2549) [VM/VMSS] Improved disk caching support (Azure#2522) VM/VMSS: incorporate credentials validation logic used by portal (Azure#2537) Script that creates packaged releases package archive (Azure#2508) Adding alias for defaults flag (Azure#2540) Add wait commands and --no-wait support (Azure#2524) choice list outside of named arguments (Azure#2521) Fixed test failure in test_sql_db_mgmt. (Azure#2530) core: support login using service principal with a cert (Azure#2457) Add note about being in preview (Azure#2512) vm:fix distro check mechanism used by disk encryption (Azure#2511) ...
Adds
wait
command for the following:express-route, local-gateway, vnet-gateway, vmss
Add
--no-wait
parameters to the following:vm:
create, delete, deallocate, generalize, stop, start, restart, redeploy, resize
vm user:
update, delete, reset-ssh
vmss:
create, delete, update, deallocate, delete-instances, stop, restart, start, update-instances, reimage, scale
vnet-gateway:
delete
local-gateway:
create, delete, update
express-route:
create, delete, update