Add prompting for missing template parameters. #2364

brendandburns · 2017-03-03T06:26:05Z

codecov-io · 2017-03-04T03:52:40Z

Codecov Report

Merging #2364 into master will decrease coverage by 0.19%.
The diff coverage is 28.43%.

@@            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

Impacted Files	Coverage Δ
src/azure-cli-core/azure/cli/core/prompting.py	`30.12% <23.52%> (-6.05%)`	❌
...ource/azure/cli/command_modules/resource/custom.py	`54.33% <33.33%> (-2.85%)`	❌
...dback/azure/cli/command_modules/feedback/custom.py	`34.69% <0%> (ø)`	✅
src/azure-cli-core/azure/cli/core/_util.py	`66.66% <0%> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 242874d...2c49eba. Read the comment docs.

derekbekoe · 2017-03-06T18:19:59Z

src/azure-cli-core/azure/cli/core/prompting.py

+        try:
+            return int(value)
+        except ValueError:
+            print('{} is not a valid number'.format(value))


We suggest using logger.warning instead of printing to stdout.

Same with the other uses here.

done for the warnings, keeping print for the help string since that's not really a warning...

derekbekoe · 2017-03-06T18:23:07Z

src/azure-cli-core/azure/cli/core/prompting.py

+        val = input(msg)
+        if val == '?' and help_string is not None:
+            print(help_string)
+            continue


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?

If we do want this, we should add the message ' (? for help)' in here to msg if help_string != None

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.

okay that works

derekbekoe · 2017-03-06T18:29:12Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -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):


With this, prompting is on by default.
In #1778 (comment), looks like the request was to only prompt if --prompt is supplied.

tjprescott · 2017-03-06T18:46:10Z

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?

brendandburns · 2017-03-08T04:22:02Z

@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\"}}"

brendandburns · 2017-03-08T04:27:40Z

Comments mostly addressed, please re-check.

tjprescott

A couple changes requested and please post an example of what the output looks like when it is in prompting mode.

tjprescott · 2017-03-08T18:06:09Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py


 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):


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)?

@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..

tjprescott · 2017-03-08T18:07:20Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -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:


We should do the TTY check here and automatically fail if no TTY is detected (see my earlier comment).

the prompt_ family of functions have that check already built into them, I can add it here, but it seems redundant.

Nope, if it's in the prompt methods, then that's sufficient.

brendandburns · 2017-03-09T00:10:57Z

@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):

troydai · 2017-03-10T05:00:54Z

...ure-cli-resource/azure/cli/command_modules/resource/tests/test_resource_list_odata_filter.py

@@ -4,9 +4,45 @@
 # --------------------------------------------------------------------------------------------


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.

brendandburns · 2017-03-10T17:28:04Z

@tjprescott @derekbekoe can you folks decide which behavior (default prompt vs default non-prompt) you want, and I'll implement?

derekbekoe

@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.

derekbekoe · 2017-03-09T01:54:15Z

src/azure-cli-core/azure/cli/core/prompting.py

+        val = input(msg)
+        if val == '?' and help_string is not None:
+            print(help_string)
+            continue


okay that works

tjprescott · 2017-03-10T18:58:45Z

@brendandburns agree with @derekbekoe. Flag isn't necessary for any reasonable use case.

brendandburns · 2017-03-11T00:27:10Z

Ok, I removed prompt_for_params please take another look.

--brendan

troydai · 2017-03-15T16:17:36Z

src/azure-cli-core/azure/cli/core/prompting.py



-def prompt_y_n(msg, default=None):
+def _prompt_bool(msg, true_str, false_str, default=None, help_string=None):


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.

msftclas added the cla-not-required label Mar 3, 2017

brendandburns force-pushed the params branch 3 times, most recently from 2d6169a to cce6005 Compare March 4, 2017 03:45

derekbekoe reviewed Mar 6, 2017

View reviewed changes

derekbekoe added the Resource Manager label Mar 7, 2017

tjprescott suggested changes Mar 8, 2017

View reviewed changes

brendandburns force-pushed the params branch from 2119e33 to d9d9536 Compare March 9, 2017 00:11

troydai reviewed Mar 10, 2017

View reviewed changes

derekbekoe reviewed Mar 10, 2017

View reviewed changes

brendandburns force-pushed the params branch from d9d9536 to 10c65f9 Compare March 11, 2017 00:26

brendandburns mentioned this pull request Mar 11, 2017

Add support for 'OptionalParameters' to group deployment create|validate #2463

Closed

derekbekoe approved these changes Mar 13, 2017

View reviewed changes

tjprescott approved these changes Mar 14, 2017

View reviewed changes

Add prompting for missing template parameters.

2c49eba

brendandburns force-pushed the params branch from 10c65f9 to 2c49eba Compare March 14, 2017 23:49

troydai reviewed Mar 15, 2017

View reviewed changes

troydai approved these changes Mar 15, 2017

View reviewed changes

troydai merged commit c2b1a92 into Azure:master Mar 15, 2017

mozehgir added the ARM az resource/group/lock/tag/deployment/policy/managementapp/account management-group label Aug 14, 2019

haroldrandom added cla-not-required Resource Manager labels Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prompting for missing template parameters. #2364

Add prompting for missing template parameters. #2364

brendandburns commented Mar 3, 2017

codecov-io commented Mar 4, 2017 •

edited

derekbekoe Mar 6, 2017

derekbekoe Mar 6, 2017

brendandburns Mar 8, 2017

derekbekoe Mar 6, 2017

derekbekoe Mar 6, 2017

brendandburns Mar 8, 2017

derekbekoe Mar 9, 2017

derekbekoe Mar 6, 2017

brendandburns Mar 8, 2017

tjprescott commented Mar 6, 2017

brendandburns commented Mar 8, 2017

brendandburns commented Mar 8, 2017

tjprescott left a comment

tjprescott Mar 8, 2017

brendandburns Mar 9, 2017

tjprescott Mar 8, 2017

brendandburns Mar 9, 2017

tjprescott Mar 9, 2017

brendandburns commented Mar 9, 2017

troydai Mar 10, 2017

brendandburns commented Mar 10, 2017

derekbekoe left a comment

derekbekoe Mar 9, 2017

tjprescott commented Mar 10, 2017

brendandburns commented Mar 11, 2017

troydai Mar 15, 2017 •

edited

		@@ -4,9 +4,45 @@
		# --------------------------------------------------------------------------------------------



		def prompt_y_n(msg, default=None):
		def _prompt_bool(msg, true_str, false_str, default=None, help_string=None):

Add prompting for missing template parameters. #2364

Add prompting for missing template parameters. #2364

Conversation

brendandburns commented Mar 3, 2017

codecov-io commented Mar 4, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Mar 6, 2017

brendandburns commented Mar 8, 2017

brendandburns commented Mar 8, 2017

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Mar 9, 2017

Choose a reason for hiding this comment

brendandburns commented Mar 10, 2017

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Mar 10, 2017

brendandburns commented Mar 11, 2017

troydai Mar 15, 2017 • edited

Choose a reason for hiding this comment

codecov-io commented Mar 4, 2017 •

edited

troydai Mar 15, 2017 •

edited