-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 prompting for missing template parameters. #2364
Conversation
2d6169a
to
cce6005
Compare
Codecov Report
@@ Coverage Diff @@
## master #2364 +/- ##
=========================================
- Coverage 72.47% 72.28% -0.2%
=========================================
Files 362 362
Lines 19582 19669 +87
Branches 2871 2896 +25
=========================================
+ Hits 14192 14217 +25
- Misses 4477 4534 +57
- Partials 913 918 +5
Continue to review full report at Codecov.
|
try: | ||
return int(value) | ||
except ValueError: | ||
print('{} is not a valid number'.format(value)) |
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 suggest using logger.warning
instead of printing to stdout.
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.
Same with the other uses here.
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.
done for the warnings, keeping print for the help string since that's not really a warning...
val = input(msg) | ||
if val == '?' and help_string is not None: | ||
print(help_string) | ||
continue |
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.
This pattern of using ?
to show more info on the prompt is new to me and not used anywhere else in the CLI.
Is there precedent for this?
Also @Azure/azure-cli thoughts on this?
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 we do want this, we should add the message ' (? for help)' in here to msg
if help_string != None
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.
Yeah, it's up to you.
I did it because it's awkward and repetitive for users who know what they are doing to always see the help text (and the text can be super long, so it can mess up formatting)
But templates have built in descriptions for parameters, so it seemed really useful to surface those to users somehow.
I defer to your judgement about the best way to do this.
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.
okay that works
@@ -93,18 +94,70 @@ def export_group_as_template( | |||
|
|||
def deploy_arm_template( | |||
resource_group_name, template_file=None, template_uri=None, deployment_name=None, | |||
parameters=None, mode='incremental', no_wait=False): | |||
parameters=None, mode='incremental', no_wait=False, prompt_for_parameters=True): |
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.
With this, prompting is on by default.
In #1778 (comment), looks like the request was to only prompt if --prompt
is supplied.
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.
done.
Could you provide a print of the new -h output for the revised command, as well as an example of the output the command produces when prompting for deployment parameters? |
@tjprescott here's the new help: Command
az group deployment create: Start a deployment.
Arguments
--resource-group -g [Required]: Name of resource group.
--mode : Incremental (only add resources to resource group) or Complete
(remove extra resources from resource group). Allowed values:
Complete, Incremental. Default: incremental.
--name -n : The deployment name. Default to template file base name.
--no-wait : Do not wait for the long running operation to finish.
--parameters : Provide deployment parameter values, either json string, or use
'@<file path>' to load from a file.
--prompt : If true, prompt for parameters in the template that are missing
values.
--template-file : A template file path in the file system.
--template-uri : A uri to a remote template file.
Global Arguments
--debug : Increase logging verbosity to show all debug logs.
--help -h : Show this help message and exit.
--output -o : Output format. Allowed values: json, jsonc, table, tsv.
Default: json.
--query : JMESPath query string. See http://jmespath.org/ for more
information and examples.
--verbose : Increase logging verbosity. Use --debug for full debug logs.
Examples
Create a deployment from a remote template file.
az group deployment create -g MyResourceGroup --template-uri
https://myresource/azuredeploy.json --parameters @myparameters.json
Create a deployment from a local template file and use parameter values in a string.
az group deployment create -g MyResourceGroup --template-file azuredeploy.json --parameters
"{\"location\": {\"value\": \"westus\"}}" |
Comments mostly addressed, please re-check. |
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.
A couple changes requested and please post an example of what the output looks like when it is in prompting mode.
|
||
def validate_arm_template(resource_group_name, template_file=None, template_uri=None, | ||
parameters=None, mode='incremental'): | ||
parameters=None, mode='incremental', prompt_for_parameters=False): |
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.
It seems to me we tend to opt-out of interactivity rather than opt-in, which would make this a --suppress-prompt flag instead of the opposite. One could argue that this flag shouldn't be needed at all. If you are in interactive mode, you probably don't want to type --prompt to get the prompting behavior, and if you are running a script, it should just detect the lack of TTY and fail if the parameters aren't provided. In that case, when would you ever need to use --suppress-prompt (if it existed)?
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.
@tjprescott @derekbekoe requested --prompt
here: #2364 (comment)
I don't care which we do, but you folks should agree between yourselves and then let me know..
@@ -122,6 +175,13 @@ def _deploy_arm_template_core(resource_group_name, template_file=None, template_ | |||
else: | |||
template = get_file_json(template_file) | |||
|
|||
missing = _find_missing_parameters(parameters, template) | |||
if len(missing) > 0: | |||
if prompt_for_parameters: |
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 should do the TTY check here and automatically fail if no TTY is detected (see my earlier comment).
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.
the prompt_
family of functions have that check already built into them, I can add it here, but it seems redundant.
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.
Nope, if it's in the prompt methods, then that's sufficient.
@tjprescott Here's the example output: > .\az group deployment create --template-file=..\azuredeploy.json --prompt -g foo
Please provide a value for 'sshRSAPublicKey' (? for help): ?
SSH public key used for auth to all Linux machines. Not Required. If not set, you must provide a password key.
Please provide a value for 'sshRSAPublicKey' (? for help): fakekey
Please provide a value for 'linuxAdminUsername' (? for help): fakeuser
Please provide a value for 'clientPrivateKey' (? for help):
The base 64 client private key used to communicate with the master
Please provide a value for 'clientPrivateKey' (? for help):
Please provide a value for 'apiServerCertificate' (? for help): |
@@ -4,9 +4,45 @@ | |||
# -------------------------------------------------------------------------------------------- |
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.
Please note that the test files have been moved. You need to rebase on the last master and apply the changes to the test files are new location.
@tjprescott @derekbekoe can you folks decide which behavior (default prompt vs default non-prompt) you want, and I'll implement? |
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.
@brendandburns Recommendation is to remove prompt_for_parameters
. This way, if TTY is available, the command will prompt for missing params. If TTY is not available, prompting will fail (due to TTY check) and the command will fail as expected.
val = input(msg) | ||
if val == '?' and help_string is not None: | ||
print(help_string) | ||
continue |
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.
okay that works
@brendandburns agree with @derekbekoe. Flag isn't necessary for any reasonable use case. |
Ok, I removed --brendan |
|
||
|
||
def prompt_y_n(msg, default=None): | ||
def _prompt_bool(msg, true_str, false_str, default=None, help_string=None): |
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.
Prompting "yes" or "no" essentially is just a particular case prompting multiple choices. In my opinion, we can combine these two methods. However, I do not intend to block this PR since the change as it works fine for now.
Fixes #1778