Support bool default in AzdMetadata and use Confirmation for boolean parameters
#5384
Conversation
Menghua1
requested review from
wbreza,
vhvb1989,
hemarina,
weikanglim,
JeffreyCA and
rajeshkamal5050
as code owners
There was a problem hiding this comment.
LGTM - @vhvb1989, will adding this flexibility cause any unintended issues elsewhere?
|
FYI @alexwolfmsft for updating the docs |
| defaultStr := fmt.Sprintf("%v", azdMetadata.Default) | ||
| tmp := defaultStr | ||
| return &tmp |
There was a problem hiding this comment.
| defaultStr := fmt.Sprintf("%v", azdMetadata.Default) | |
| tmp := defaultStr | |
| return &tmp | |
| defaultStr := fmt.Sprintf("%v", azdMetadata.Default) | |
| return &defaultStr |
| defaultBool := true | ||
| if azdMetadata.Default != nil { | ||
| switch v := azdMetadata.Default.(type) { | ||
| case bool: | ||
| defaultBool = v | ||
| case string: | ||
| defaultBool = strings.EqualFold(v, "true") | ||
| default: | ||
| strVal := fmt.Sprintf("%v", v) | ||
| defaultBool = strings.EqualFold(strVal, "true") | ||
| } | ||
| } | ||
| msg := fmt.Sprintf("Would you like to set the '%s' infrastructure %s to %t?", key, securedParam, defaultBool) | ||
| confirmValue, err := p.console.Confirm(ctx, input.ConsoleOptions{ | ||
| Message: msg, | ||
| Help: help, | ||
| Options: options, | ||
| DefaultValue: defaultValueForPrompt, | ||
| DefaultValue: true, | ||
| }) |
There was a problem hiding this comment.
I feel like the new prompt change could get a bit confusing, especially if the Bicep param has a default of false because of the double negative.
Bicep:
@metadata({azd: {
default: false
}})
param param1 boolNew prompt: if the user inputs 'n', then the value becomes true in this case:
Old prompt, a bit more explicit:
@SophCarp would love to get your thoughts here as well 🙂
There was a problem hiding this comment.
I prefer the way of selection (y/n) in the new prompt, but it does feel confusing due to double negative. Could we ask if users would like to set 'paraml' to true (y/N)?
There was a problem hiding this comment.
hmmm. By default, param1 would be false. y/n isn't the same as true/false, but rather "confirm/change". "default" should be included in the question, because otherwise a customer might be confused why they're being asked a double negative question.
Maybe :
? 'param1's default value is false. Keep default value? y/nor:
? Infrastructure parameter 'param1' has a default value of 'false'. Keep default value?
> Keep (param1=false)
Change (param1=true) or:
? Confirm infrastructure parameter 'param1' = false:
> Yes (param1=false)
No (param1=true)or:
? Confirm infrastructure parameter 'param1' = false: y/n
Is there more direct wording than "infrastructure parameter"? This is different from "environment variable", right?
|
used #5386 to fix the prompt regressions on bool/int with no defaults. @Menghua1 , let's use this PR for only adding support for types in the default azd metadata field.
Instead of the current:
Similar for |
Fix #5276. This PR includes the following changes:
defaultfield type in the AzdMetadata structure toanyto support bothboolandstring.boolparameters, use Confirmation (Yes/No options) instead ofsurvey.Select.@rajeshkamal5050 and @vhvb1989 for notification.