core: support setting default values for common arguments like default resource group, default web, default vm #2414
Conversation
yugangw-msft
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2414 +/- ##
==========================================
- Coverage 72.47% 72.44% -0.04%
==========================================
Files 361 362 +1
Lines 19509 19547 +38
Branches 2856 2865 +9
==========================================
+ Hits 14139 14160 +21
- Misses 4463 4473 +10
- Partials 907 914 +7
Continue to review full report at Codecov.
|
|
This is really awesome to see this experience moving forward! I just had a few thoughts on this specific proposal:
Suggestion #3 and #4 would effectively allow translating your sample code: az group create -n yugangw3 -l eastus
az appservice set-defaults --resource-group yugangw3
az appservice plan create -n yugangw3-plan --sku s1
az appservice plan show -n yugangw3-plan
az appservice web create --plan yugangw3-plan -n yugangw3-web
az appservice set-defaults --webapp yugangw3-web
az appservice web showInto the following (note the need to configure the SKU and location as part of the web app, since you're not referencing an existing plan/group): $ az appservice web create -n yugangw3-web -s s1 -l eastus
az appservice web showMost of this could presumably be layered on top of the work you're enabling in this proposal, but it would be great to determine what degree of opinion you feel is appropriate for Az CLI, as opposed to a derivative CLI that could choose to be more prescriptive. |
|
@lostintangent. Thanks for the suggestion. |
|
@yugangw-msft Makes sense. I recognize there are issues for all of these items, I just wanted to mention them since you mentioned not believing that #1771 would be necessary, and I wanted to provide my opinion about that and surrounding features for simplifying this E2E. Thanks! |
|
@yugangw-msft How will this look now to set the default RG via the Will this be a way to discover the set of known name/value pairs that can be defaulted via the |
az configure --default-resource-group my RG
az configure --section appservice --name default_webapp --value myweb1
The discoverability was the main reason I exposed (env) D:\sdk\azure-cli>az appservice web show -h
Command
az appservice web show: Show a web app.
Arguments
--slot -s : The name of the slot. Default to the productions slot if not specified.
Resource Id Arguments
--ids : One or more resource IDs (space delimited). If provided, no other 'Resource
Id' arguments should be specified.
--name -n : Name of the web. You can configure the default web using 'az configure
--section appservice --name default_webapp'. |
|
Great, thanks! The Just thinking out loud, the following workflow feels simple and natural to me (collapsing the section/name into a single slashed value, removing the $ az configure --setting appservice/webapp --value myweb1
# With shorthand aliases
$ az configure -s appservice/webapp -v webapp1Either way, this is an awesome PR. I'm just thinking out loud since you mentioned this was a prototype for discussion :) |
@lostintangent I like this suggestion. Behind the scenes, it gets saved in the config like this which makes sense to me: I came up with the idea of |
|
@yugangw-msft Are we making a special case expection for resource group having it's own param name like this?
What if we have |
|
@derekbekoe I like that RG suggestion! |
| - name: configure a default webapp | ||
| text: > | ||
| az configure --section appservice --name default_webapp_name --value myweb1 | ||
| az configure --section compute/vm --value myvm |
There was a problem hiding this comment.
Should this be "--setting"?
| configure common settings | ||
| :param str section: configuration section | ||
| :param str name: configuration variable name | ||
| :param str setting: configuration variable, e.g. resource-group, appservice/webapp, compute/vm |
There was a problem hiding this comment.
The example here uses "resource-group", whereas it looks like it's actually "resource_group". I actually prefer the hyphenated version, since that seems to be consistent with what the CLI currently uses for word separation.
There was a problem hiding this comment.
Actually, in order to match the name of the existing noun in the CLI, could we just call this setting "group"?
There was a problem hiding this comment.
I initially used resource-group, but switched to resource_group, as the existing variables there all use _ style. I would leave to @derekbekoe for final comment.
Regarding to group, I am fine with that with the only concern that by appearing in the global config, the meaning is not easy to grab, as the group could mean lots of other things
There was a problem hiding this comment.
Since we are here, should we support to set multiple values at the same time? Say use a space separated name=value pairs
--settings resource_group=rg1 appservice/web=myweb compute/vm=myvm1
There was a problem hiding this comment.
I was literally just thinking the same thing! :) Yeah this would be great, so that you could easily set the group and VM/web app at the same time.
There was a problem hiding this comment.
The actual setting saved to config should be underscore cased but the command could accept both underscore and dashed forms?
|
|
||
| register_cli_argument('appservice web', 'slot', options_list=('--slot', '-s'), help="the name of the slot. Default to the productions slot if not specified") | ||
| register_cli_argument('appservice web', 'name', configured_default='appservice/default_webapp_name', | ||
| register_cli_argument('appservice web', 'name', configured_default='appservice/webapp', |
There was a problem hiding this comment.
What do you think about calling this "appservice/web" in order to match the noun already used in the CLI? I like the consistency, and it feels a little more intuitive.
| az configure --setting resource_group myRG | ||
| - name: configure a default webapp | ||
| text: > | ||
| az configure --section compute/vm --value myvm |
There was a problem hiding this comment.
For the same reason I mentioned for group and appservice/web, I'd love to discuss whether this could just be "vm" instead of "compute/vm", since the former would match the "resource hierarchy" of the respective noun in the CLI.
From a discoverability perspective, having the setting names match their corresponding command group structure seems like it could help make them more predictable.
There was a problem hiding this comment.
Having just 'vm' would mean it gets put in 'core':
[core]
vm = myvm
IMO that doesn't look quite right as it shouldn't be under core.
Suggestion, 'vm/name':
[vm]
name = myvm
export AZURE_VM_NAME=myvm
There was a problem hiding this comment.
I thought about that yesterday as well.
For this feature, there is no good discoverability, because the whole process is opt-in, so you will need to check out the help on the argument to know whether this arg's default value can be configured. That is why I initially exposed a strong typed command of set-defaults
That said, I still think we can do some change to get closer to simplicity. Right now 'resource_group' goes to [core] section with other configs, and "appservice/webapp" goes to [appservice] section, I am thinking maybe we can just use a section like [default], and put in all those values there? This way, it is fine to just use group. The configure command can be simplified az configure --settings vm=vm1 group=myrg web=myweb.
| register_cli_argument('appservice', 'resource_group_name', arg_type=resource_group_name_type) | ||
| register_cli_argument('appservice', 'location', arg_type=location_type) | ||
|
|
||
| register_cli_argument('appservice set-defaults', 'clear_all', action='store_true') |
There was a problem hiding this comment.
We don't need this anymore right?
| register_cli_argument('appservice web create', 'name', options_list=('--name', '-n'), help='name of the new webapp') | ||
| register_cli_argument('appservice web', 'name', configured_default='appservice/webapp', | ||
| arg_type=name_arg_type, completer=get_resource_name_completion_list('Microsoft.Web/sites'), id_part='name', | ||
| help="name of the web. You can configure the default web using 'az configure --section appservice --name default_webapp_name'") |
There was a problem hiding this comment.
I believe the help is incorrect now the configure command has changed.
| examples: | ||
| - name: configure a default resource group | ||
| text: > | ||
| az configure --setting resource_group myRG |
There was a problem hiding this comment.
IMO in the help, this should be resource-group instead of resource_group.
But in the actual config file, it should be saved in underscore casing.
So in your logic you can do str.replace('-', '_'). Then you could actually support both - and _ with little effort.
There was a problem hiding this comment.
Agreed. Like i said, I am thinking we just use a dedicated section [default], and use group
There was a problem hiding this comment.
Per our conversation, let us not do the character replacement which will cause inconsistency when users display existing configurations. So I will do 2 things
- Let go with
group - We will
tryto align with the top categories, which will give usvm/name,vmss/name,appservice/webname.
| az configure --setting resource_group myRG | ||
| - name: configure a default webapp | ||
| text: > | ||
| az configure --section compute/vm --value myvm |
There was a problem hiding this comment.
Having just 'vm' would mean it gets put in 'core':
[core]
vm = myvm
IMO that doesn't look quite right as it shouldn't be under core.
Suggestion, 'vm/name':
[vm]
name = myvm
export AZURE_VM_NAME=myvm
| setattr(arg.type, 'configured_default_applied', True) | ||
| parts = def_config.split('/') | ||
| section = 'core' if len(parts) == 1 else parts[0] | ||
| config_name = parts[-1] |
There was a problem hiding this comment.
The 3 lines above should go into _config.py in a helper method.
In the PR, similar logic is used in configure/custom.py.
|
|
||
| return | ||
|
|
||
| # if nothing supplifed, we go interactively |
There was a problem hiding this comment.
Typo supplifed.
Not sure if you wanted supplied or specified 😄
There was a problem hiding this comment.
Guess both crossed my mind at the same time. Thanks.
| :param str value: configuration variable value | ||
| ''' | ||
| if setting or value: | ||
| if bool(setting) != bool(value): |
There was a problem hiding this comment.
supposed to verify both values are true :). Let me get it clearer by using if not setting or not value:
|
|
||
| def _normalize_config_value(value): | ||
| if value: | ||
| value = '' if value in ["''", '""'] else value |
There was a problem hiding this comment.
Is this to remove the default?
| register_cli_argument('group export', 'include_comments', action='store_true') | ||
| register_cli_argument('group export', 'include_parameter_default_value', action='store_true') | ||
| register_cli_argument('group create', 'resource_group_name', completer=None) | ||
| register_cli_argument('group create', 'rg_name', options_list=('--name', '-n'), help='name of the new resource group', completer=None) |
There was a problem hiding this comment.
This doesn't use resource_group_name_type in parameters.py.
Does that mean the default RG won't apply here?
There was a problem hiding this comment.
yes, for create, we should not use the default RG
There was a problem hiding this comment.
FYI, for other renaming on vm/vmss/vmss, my recent push has got rid of it. I decided to handle them in one place, that for create command, the name part will not load the default configured value. This is not a new thing, we did the same on splitting id-part
| # VM CREATE PARAMETER CONFIGURATION | ||
|
|
||
| register_cli_argument('vm create', 'name', name_arg_type, validator=_resource_not_exists('Microsoft.Compute/virtualMachines')) | ||
| register_cli_argument('vm create', 'new_vm_name', options_list=('--name', '-n'), metavar='NAME', |
There was a problem hiding this comment.
Q: Why did you need to change to new_vm_name?
There was a problem hiding this comment.
So that this arg name would not pick the default from config, and we only need to do it for create, It appears the arg matching we have is based on string match. I can special case in the core layer to skip any create command like we did in other places, but again since it is limited on create command, so I do it at command module layer
|
@yugangw-msft @derekbekoe Personally, I'm not entirely in love with the new What if we renamed the Maybe it's just me, but I really like the idea of the "key path" matching the same hierarchy as the CLI itself. It just feels very clean. With this change, the use of the command also "reads" pretty naturally IMO (E.g. "Configure the default group to be 'foo' and the default vm to be 'bar'"). az configure --defaults group=foo vm=bar
az configure --defaults appservice/web=baz |
|
@lostintangent, |
|
@yugangw-msft Sounds good. I was by no means partial to the sub-section vs. key prefixing solution, I was just throwing out one approach, that may or may be simpler based on how you're actually persisting/loading the config data. Thanks so much for having this awesome open discussion! |
|
LGTM! |
6 participants
This is a prototype, but sufficient for review to get the ball rolling.
Per conversation with @johanste, @derekbekoe, the mechanisms will be simple. A few notes:
~/.azure/config, or you can use environment variablesrequiredargument, not for optional arg. This wayvm list/'appservice web list' will still work to list across the subscription--ids, I will still respect it, but I made a change of not to emit a warning if theidconflicts with the default resource group.For reference, the command flow will look like