feat: Remove disabled fields from showing on redeployment in server mode #839

philasmar · 2024-07-02T16:52:52Z

Issue #, if available:
DOTNET-6316

Description of changes:

Added a 'VisibileOnRedeployment' boolean on the option settings in the Recipe which determines the visibility of a setting during a redeployment when 'Updatable' is false
The setting is false by default, meaning any setting with 'Updatable' = false will not be visible during redeployment.
I have set the fields that have important names as visible during redeployment
I have tested this behavior with the toolkit and CLI

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

awschristou · 2024-07-02T17:03:56Z

src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs

-                    Visible = optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting),
+                    Visible =
+                        optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting) &&
+                        !(recommendation.IsExistingCloudApplication && !setting.Updatable && !setting.VisibleOnRedeployment),


can test coverage be applied to this new logic

"visible on redeployment" sounds definitive, but this isn't the case. It appears that "Updatable" takes priority (which makes sense). How can future maintainers avoid assumptions when they set VisibleOn to true and expect their field to always appear?

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.

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.

Initially my changes were in IsOptionSettingDisplayable but 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.

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.

Added a comment

codecov · 2024-07-02T17:33:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.65%. Comparing base (2c676af) to head (3759495).
Report is 7 commits behind head on dev.

Additional details and impacted files

@@             Coverage Diff             @@
##              dev     #839       +/-   ##
===========================================
+ Coverage   32.41%   61.65%   +29.24%     
===========================================
  Files         272      278        +6     
  Lines       10746    10811       +65     
  Branches     1482     1492       +10     
===========================================
+ Hits         3483     6666     +3183     
+ Misses       6974     3609     -3365     
- Partials      289      536      +247

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ashovlin · 2024-07-09T15:23:04Z

src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs

-                    Visible = optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting),
+                    Visible =
+                        optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting) &&
+                        !(recommendation.IsExistingCloudApplication && !setting.Updatable && !setting.VisibleOnRedeployment),
                    SummaryDisplayable = optionSettingHandler.IsSummaryDisplayable(recommendation, setting),


Could you hide these from the summary as well? This pane still has the "Create new" language that kicked this off.

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 Edit settings they will only see the ones that are editable as well as the disabled ones that we want visible.

The highlighted Create New is specifically what was called out first in aws/aws-toolkit-visual-studio#429, so I still think it'd be great to get it out of the summary. The "Create New Roles" too.

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 Create New and similar options from the summary view, I need to add custom logic which I prefer to treat as a separate effort.

ashovlin · 2024-07-09T15:24:22Z

src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs

-                    Visible = optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting),
+                    Visible =
+                        optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting) &&
+                        !(recommendation.IsExistingCloudApplication && !setting.Updatable && !setting.VisibleOnRedeployment),


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.

ashovlin · 2024-07-09T16:01:16Z

src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppAppRunner.recipe

@@ -100,6 +100,7 @@
            "TypeHint": "AppRunnerService",
            "AdvancedSetting": false,
            "Updatable": false,
+            "VisibleOnRedeployment": true,


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.

AppRunner - KMS key

Fargate - VPC Id, Load Balancer ARN

Beanstalk - IAM role ARN, EC2 key pair

But not a deal breaker if anyone feels strongly, and this is something we can toggle as needed.

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.

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 Updatable is set to false and VisibleOnRedeployment is set to true

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.

96malhar · 2024-07-10T19:34:32Z

src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppAppRunner.recipe

@@ -100,6 +100,7 @@
            "TypeHint": "AppRunnerService",
            "AdvancedSetting": false,
            "Updatable": false,
+            "VisibleOnRedeployment": true,


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 Updatable is set to false and VisibleOnRedeployment is set to true

ashovlin

Good with this as an incremental improvement, but I still think we should try to get "Create" out of the summary pane before fully resolving aws/aws-toolkit-visual-studio#429

philasmar · 2024-07-15T20:21:22Z

Good with this as an incremental improvement, but I still think we should try to get "Create" out of the summary pane before fully resolving aws/aws-toolkit-visual-studio#429

I will create a backlog item to remove the "Create" out of the summary pane.

philasmar requested review from ashovlin, normj and 96malhar July 2, 2024 16:52

awschristou reviewed Jul 2, 2024

View reviewed changes

feat: Remove disabled fields from showing on redeployment in server mode

fdf67d3

philasmar force-pushed the asmarp/remove-disabled-settings-servermode branch from 1e650ae to fdf67d3 Compare July 2, 2024 20:17

ashovlin reviewed Jul 9, 2024

View reviewed changes

96malhar requested changes Jul 10, 2024

View reviewed changes

make more settings visible

0fa1201

philasmar requested review from 96malhar and ashovlin July 11, 2024 14:19

fix integ test

3759495

96malhar approved these changes Jul 12, 2024

View reviewed changes

ashovlin approved these changes Jul 15, 2024

View reviewed changes

philasmar merged commit db9ec45 into dev Jul 15, 2024
11 checks passed

philasmar deleted the asmarp/remove-disabled-settings-servermode branch July 15, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Remove disabled fields from showing on redeployment in server mode #839

feat: Remove disabled fields from showing on redeployment in server mode #839

philasmar commented Jul 2, 2024

awschristou Jul 2, 2024

philasmar Jul 2, 2024

ashovlin Jul 9, 2024

philasmar Jul 10, 2024

ashovlin Jul 10, 2024

philasmar Jul 11, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

ashovlin Jul 9, 2024

philasmar Jul 10, 2024

ashovlin Jul 10, 2024

philasmar Jul 11, 2024

ashovlin Jul 9, 2024

ashovlin Jul 9, 2024 •

edited

Loading

philasmar Jul 10, 2024

96malhar Jul 10, 2024 •

edited

Loading

philasmar Jul 11, 2024

96malhar Jul 10, 2024 •

edited

Loading

ashovlin left a comment

philasmar commented Jul 15, 2024

feat: Remove disabled fields from showing on redeployment in server mode #839

feat: Remove disabled fields from showing on redeployment in server mode #839

Conversation

philasmar commented Jul 2, 2024

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

codecov bot commented Jul 2, 2024 • edited Loading

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

ashovlin Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

96malhar Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

96malhar Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

ashovlin left a comment

Choose a reason for hiding this comment

philasmar commented Jul 15, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

ashovlin Jul 9, 2024 •

edited

Loading

96malhar Jul 10, 2024 •

edited

Loading

96malhar Jul 10, 2024 •

edited

Loading