-
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
[AKS] improve tab completion for VM sizes #5393
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5393 |
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.
One question.
try: | ||
location = namespace.location | ||
except AttributeError: | ||
location = get_one_of_subscription_locations(cmd.cli_ctx) | ||
result = get_vm_sizes(cmd.cli_ctx, location) | ||
return [r.name for r in result] | ||
return sorted(list(set(r.name for r in result) & set(c.value for c in ContainerServiceVMSizeTypes))) |
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.
If ContainerServiceVMSizeTypes is a static enum, why not just add it as an enum_type instead of doing an API call?
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.
You mean configure the argument in _params.py with something like get_enum_type(vm_size_types)
? Seems like that should be added too. I was just trying to make minimal changes since this completer is shared by three arguments across az acs
and az aks
.
How does that interact with completer=get_vm_size_completion_list
? The behavior in this PR is consistent with before--the list of VM sizes is dynamic, depending on a --location
argument or the user's subscription locations.
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.
Ah, okay, that makes sense.
Currently, hitting [TAB] for a
--vm-size
-type argument returns all VM sizes available for a location, as returned by the Compute Service. But AKS only supports a subset of those sizes, so the list can be misleading.This changes the
get_vm_size_completion_list
completer function to return the intersection of the VM sizes allowed by the ACS SDK with those returned by the Compute Service.