Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Jul 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fdf67d3
feat: Remove disabled fields from showing on redeployment in server mode
philasmar Jul 1, 2024
0fa1201
make more settings visible
philasmar Jul 11, 2024
3759495
fix integ test
philasmar Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,9 @@
Value = optionSettingHandler.GetOptionSettingValue(recommendation, setting),
Advanced = setting.AdvancedSetting,
ReadOnly = recommendation.IsExistingCloudApplication && !setting.Updatable,
Visible = optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting),
Visible =
optionSettingHandler.IsOptionSettingDisplayable(recommendation, setting) &&
!(recommendation.IsExistingCloudApplication && !setting.Updatable && !setting.VisibleOnRedeployment),

Check warning on line 213 in src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs

View check run for this annotation

Codecov / codecov/patch

src/AWS.Deploy.CLI/ServerMode/Controllers/DeploymentController.cs#L211-L213 

Added lines #L211 - L213 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment

SummaryDisplayable = optionSettingHandler.IsSummaryDisplayable(recommendation, setting),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

AllowedValues = setting.AllowedValues,
ValueMapping = setting.ValueMapping,
Expand Down
5 changes: 5 additions & 0 deletions src/AWS.Deploy.Common/Recipes/OptionSettingItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public partial class OptionSettingItem : IOptionSettingItem
/// </summary>
public bool Updatable { get; set; }

/// <summary>
/// If the value is true, the setting will be displayed during a redeployment. This only applies to server-mode clients.
/// </summary>
public bool VisibleOnRedeployment { get; set; }

/// <summary>
/// List of all validators that should be run when configuring this OptionSettingItem.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppAppRunner.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"TypeHint": "AppRunnerService",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
Copy link
Member

@ashovlin ashovlin Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@96malhar 96malhar Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": [
{
Expand Down
7 changes: 6 additions & 1 deletion src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppECSFargate.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
"TypeHint": "ECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "CreateNew",
Expand All @@ -136,6 +137,7 @@
"TypeHint": "ExistingECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand All @@ -160,6 +162,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down Expand Up @@ -195,6 +198,7 @@
"DefaultValue": "{StackName}-service",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down Expand Up @@ -738,7 +742,8 @@
"Type": "Bool",
"DefaultValue": true,
"AdvancedSetting": false,
"Updatable": false
"Updatable": false,
"VisibleOnRedeployment": true
}
]
},
Expand Down
5 changes: 5 additions & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppElasticBeanstalkLinux.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
"TypeHint": "BeanstalkApplication",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "CreateNew",
Expand All @@ -155,6 +156,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"DependsOn": [
{
"Id": "BeanstalkApplication.CreateNew",
Expand Down Expand Up @@ -187,6 +189,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"DependsOn": [
{
"Id": "BeanstalkApplication.CreateNew",
Expand Down Expand Up @@ -215,6 +218,7 @@
"Type": "Object",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "EnvironmentName",
Expand All @@ -225,6 +229,7 @@
"DefaultValue": "{StackName}-dev",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down
4 changes: 4 additions & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/ASP.NETAppElasticBeanstalkWindows.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
"TypeHint": "BeanstalkApplication",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "CreateNew",
Expand All @@ -155,6 +156,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"DependsOn": [
{
"Id": "BeanstalkApplication.CreateNew",
Expand Down Expand Up @@ -187,6 +189,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"DependsOn": [
{
"Id": "BeanstalkApplication.CreateNew",
Expand Down Expand Up @@ -216,6 +219,7 @@
"DefaultValue": "{StackName}-dev",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down
3 changes: 3 additions & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/ConsoleAppECSFargateScheduleTask.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@
"TypeHint": "ECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "CreateNew",
Expand All @@ -154,6 +155,7 @@
"TypeHint": "ExistingECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand All @@ -178,6 +180,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down
4 changes: 4 additions & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/ConsoleAppECSFargateService.recipe
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@
"TypeHint": "ECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"ChildOptionSettings": [
{
"Id": "CreateNew",
Expand All @@ -194,6 +195,7 @@
"TypeHint": "ExistingECSCluster",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand All @@ -218,6 +220,7 @@
"DefaultValue": "{StackName}",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down Expand Up @@ -247,6 +250,7 @@
"DefaultValue": "{StackName}-service",
"AdvancedSetting": false,
"Updatable": false,
"VisibleOnRedeployment": true,
"Validators": [
{
"ValidatorType": "Regex",
Expand Down
6 changes: 6 additions & 0 deletions src/AWS.Deploy.Recipes/RecipeDefinitions/aws-deploy-recipe-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,12 @@
"description": "If the setting is false the setting can not be changed during redeployment.",
"minLength": 1
},
"VisibleOnRedeployment": {
"type": "boolean",
"title": "VisibleOnRedeployment",
"description": "If the value is true, the setting will be displayed during a redeployment. This only applies to server-mode clients.",
"minLength": 1
},
"DependsOn": {
"type": "array",
"title": "",
Expand Down
82 changes: 82 additions & 0 deletions test/AWS.Deploy.CLI.IntegrationTests/ServerModeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,88 @@ public async Task GetRecommendationsWithEncryptedCredentials()
}
}

[Fact]
public async Task AppRunnerRedeployment_VisibleOnRedeploymentSettings()
{
_stackName = $"ServerModeAppRunner{Guid.NewGuid().ToString().Split('-').Last()}";

var projectPath = _testAppManager.GetProjectPath(Path.Combine("testapps", "WebAppWithDockerFile", "WebAppWithDockerFile.csproj"));
var portNumber = 4950;
using var httpClient = ServerModeHttpClientFactory.ConstructHttpClient(ServerModeUtilities.ResolveDefaultCredentials);

var serverCommand = new ServerModeCommand(_serviceProvider.GetRequiredService<IToolInteractiveService>(), portNumber, null, true);
var cancelSource = new CancellationTokenSource();

var serverTask = serverCommand.ExecuteAsync(cancelSource.Token);
try
{
var baseUrl = $"http://localhost:{portNumber}/";
var restClient = new RestAPIClient(baseUrl, httpClient);

await restClient.WaitUntilServerModeReady();

var startSessionOutput = await restClient.StartDeploymentSessionAsync(new StartDeploymentSessionInput
{
AwsRegion = _awsRegion,
ProjectPath = projectPath
});

var sessionId = startSessionOutput.SessionId;
Assert.NotNull(sessionId);

var signalRClient = new DeploymentCommunicationClient(baseUrl);
await signalRClient.JoinSession(sessionId);

var logOutput = new StringBuilder();
RegisterSignalRMessageCallbacks(signalRClient, logOutput);

var getRecommendationOutput = await restClient.GetRecommendationsAsync(sessionId);
Assert.NotEmpty(getRecommendationOutput.Recommendations);

var appRunnerRecommendation = getRecommendationOutput.Recommendations.FirstOrDefault(x => string.Equals(x.RecipeId, "AspNetAppAppRunner"));
Assert.NotNull(appRunnerRecommendation);

await restClient.SetDeploymentTargetAsync(sessionId, new SetDeploymentTargetInput
{
NewDeploymentName = _stackName,
NewDeploymentRecipeId = appRunnerRecommendation.RecipeId
});

await restClient.StartDeploymentAsync(sessionId);

await restClient.WaitForDeployment(sessionId);

var stackStatus = await _cloudFormationHelper.GetStackStatus(_stackName);
Assert.Equal(StackStatus.CREATE_COMPLETE, stackStatus);

Assert.True(logOutput.Length > 0);

var redeploymentSessionOutput = await restClient.StartDeploymentSessionAsync(new StartDeploymentSessionInput
{
AwsRegion = _awsRegion,
ProjectPath = projectPath
});

var redeploymentSessionId = redeploymentSessionOutput.SessionId;

await restClient.SetDeploymentTargetAsync(redeploymentSessionId, new SetDeploymentTargetInput
{
ExistingDeploymentId = await _cloudFormationHelper.GetStackArn(_stackName)
});

var settings = await restClient.GetConfigSettingsAsync(redeploymentSessionId);

Assert.True(settings.OptionSettings.First(x => x.Id.Equals("ServiceName")).Visible);
Assert.False(settings.OptionSettings.First(x => x.Id.Equals("EncryptionKmsKey")).Visible);
}
finally
{
cancelSource.Cancel();
await _cloudFormationHelper.DeleteStack(_stackName);
_stackName = null;
}
}

[Fact]
public async Task WebFargateDeploymentNoConfigChanges()
{
Expand Down
Loading