feat: Remove disabled fields from showing on redeployment in server mode #839
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,15 +200,24 @@ private List<OptionSettingItemSummary> ListOptionSettingSummary(IOptionSettingHa | |
|
|
||
| foreach (var setting in configurableOptionSettings) | ||
| { | ||
| var settingSummary = new OptionSettingItemSummary( | ||
| setting.Id, | ||
| setting.FullyQualifiedId, | ||
| setting.Name, | ||
| setting.Description, | ||
| setting.Type.ToString()) | ||
| { | ||
| Category = setting.Category, | ||
| TypeHint = setting.TypeHint?.ToString(), | ||
| TypeHintData = setting.TypeHintData, | ||
| Value = optionSettingHandler.GetOptionSettingValue(recommendation, setting), | ||
| Advanced = setting.AdvancedSetting, | ||
| ReadOnly = recommendation.IsExistingCloudApplication && !setting.Updatable, | ||
| Visible = | ||
| optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting) && | ||
| // Updating visibility of settings in server-mode to be determined by 'VisibleOnRedeployment' | ||
| // when performing a redeployment and 'Updatable' is set to false. | ||
| !(recommendation.IsExistingCloudApplication && !setting.Updatable && !setting.VisibleOnRedeployment), | ||
| SummaryDisplayable = optionSettingHandler.IsSummaryDisplayable(recommendation, setting), | ||
|
There was a problem hiding this comment. Could you hide these from the summary as well? This pane still has the "Create new" language that kicked this off.
There was a problem hiding this comment. I opted to leave the summary view as is to give the customer an idea of all the settings they have already selected. However, not all of them are editable, so when they click There was a problem hiding this comment. The highlighted There was a problem hiding this comment. As discussed internally, the summary view will show all currently applied settings while the edit view will show the updatable settings plus the ones we decided should be visible. In order to remove the |
||
| AllowedValues = setting.AllowedValues, | ||
| ValueMapping = setting.ValueMapping, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,7 @@ | |
| "TypeHint": "AppRunnerService", | ||
| "AdvancedSetting": false, | ||
| "Updatable": false, | ||
| "VisibleOnRedeployment": true, | ||
|
There was a problem hiding this comment. Throughout, I'd probably be a little more conservative and leave anywhere we're displaying an ID/ARN/etc. visible if possible. In case a user is using this as a reference and then doing something else with the associated resources in the console or on another screen.
But not a deal breaker if anyone feels strongly, and this is something we can toggle as needed. There was a problem hiding this comment. Since the summary screen shows all the currently selected options, I wanted the edit screen to be mostly the updatable settings which is why I wasn't very generous with visibility of disabled settings. Curious what @normj and @96malhar think? and as you said, changing the visibility of the disabled settings is not a breaking change and we can update based on feedback. There was a problem hiding this comment. As a user, I'd prefer to always see the AWS resources associated with my application across all configuration views (including the edit screen) even if I cannot modify them. To be explicit about the immutability of certain settings on a re-deployment, we should append a line to the OptionSetting description saying - "The configured value cannot be changed during a redeployment to an existing Cloud Application". This line should be appended wherever There was a problem hiding this comment. I have made the resources that Alex mentioned visible. Also, I have experimented with adding a message to the description but it looked weird and repetetive in the toolkit. This message should be added by the Toolkit and should look different than the description so it could stand out. |
||
| "DefaultValue": "{StackName}-service", | ||
| "Validators": [ | ||
| { | ||
|
|
@@ -288,6 +289,7 @@ | |
| "Type": "String", | ||
| "AdvancedSetting": true, | ||
| "Updatable": false, | ||
| "VisibleOnRedeployment": true, | ||
| "Validators": [ | ||
| { | ||
| "ValidatorType": "Regex", | ||
|
|
||
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.
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.
I added a server mode test to mimic the behavior in the toolkit. This change only applies to server mode.
The JSON schema for the recipe explains this. I did not want to make the setting name very long.
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.
Could you also move this logic into
IsOptionSettingDisplayable? With a name like that I'd expect it to make the final determination, and wouldn't want some other caller of it to miss adding the new logic.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.
Initially my changes were in
IsOptionSettingDisplayablebut that impacts both the CLI, server mode, summary view and detailed view of option settings which is not what I want. So I moved it to server-mode specifically.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, that'd be good context to add as a comment, but not a deal breaker if you're not already revisiting this file again.
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.
Added a comment