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.

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

fix(servicecatalogappregistry): Allow user to control stack id via stack name for Application stack #24171

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

jungle-amazon
Copy link
Contributor

@jungle-amazon jungle-amazon commented Feb 14, 2023

  • Assign value of stackName to stackId for Application stack, so the stack id will always be the same as stack name. Users wanting to control stack id can do so via stackName.

Closes #24160.

Background:

  • Customers wish to control or modify the stack id of the Application stack to follow their project conventions within CDK.
  • In previous fix, we had deprecated the stack id to push users towards using stack name as the main mechanism for stack identification.

Note on backward-compatibility:

After this change, the stackId parameter can no longer be used to control the application stack id. The stack id will always match stack name, and the default stack name if not specified will be Application-${APPLICATION_IDENTIFIER}-Stack. ${APPLICATION_IDENTIFIER} is application name for CreateTargetApplication and application id for ExistingTargetApplication.

If you created an application stack prior to aws-cdk release v2.64.0 and did not specify a stack id or name, you may run into the following error during deployment due to the creation attempt of a new stack with the same application:

Resource handler returned message: "You already own an application 'MyApplicationName' (Service: ServiceCatalogAppRegistry, Status Code: 409, Request ID: xxxx)" (RequestToken: yyyy, HandlerErrorCode: InvalidRequest)

To address this error, you can set the stackName value to match your existing stack. For example:

const associatedApp = new ApplicationAssociator(app, 'MyApplicationAssociator', {
  applications: [ TargetApplication.createApplicationStack({
    applicationName: 'MyApplicationName',
    stackName: 'ApplicationAssociatorStack', // Add your existing stack name here
    ...

This will result in the existing stack deploying with the previous name, and the id in CDK will reflect this same stack name.

Related links:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Feb 14, 2023

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK feature-request A feature should be added or improved. p2 labels Feb 14, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team February 14, 2023 23:08
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@jungle-amazon jungle-amazon changed the title feat(servicecatalogappregistry): Reintroduce stack id property to allow users to define custom stack id for Application stack fix(servicecatalogappregistry): Reintroduce stack id property to allow users to define custom stack id for Application stack Feb 14, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 14, 2023 23:43

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@jungle-amazon jungle-amazon marked this pull request as ready for review February 15, 2023 00:17
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

Thanks for the extremely detailed PR description! That will be helpful to users

const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';
(this.applicationOptions.stackName as string) =
this.applicationOptions.stackName || `Application-${this.applicationOptions.applicationName}-Stack`;
const stackId = this.applicationOptions.stackId ?? `Application-${this.applicationOptions.applicationName}-Stack`;
Copy link
Contributor

Choose a reason for hiding this comment

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

stackId existed so you could change the name of the deployed stack, but that was the wrong mechanism, so we deprecated it in favor of the stackName. Now when you run cdk list or cdk deploy and you have to choose a stack id, you see a stack id that you didn't specify.

We don't need to re-introduce the stackId property to solve this; instead, we could set:

Suggested change
const stackId = this.applicationOptions.stackId ?? `Application-${this.applicationOptions.applicationName}-Stack`;
(this.applicationOptions.stackName as string) =
this.applicationOptions.stackName || `Application-${this.applicationOptions.applicationName}-Stack`;
const stackId = this.applicationOptions.stackName;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, and it was an option the issue reporter was OK with. I pushed a new commit with your proposed change to assign stack id the value of stack name. Also updated the PR description to reflect this.

@mergify mergify bot dismissed comcalvi’s stale review February 15, 2023 18:30

Pull request has been modified.

@jungle-amazon jungle-amazon changed the title fix(servicecatalogappregistry): Reintroduce stack id property to allow users to define custom stack id for Application stack fix(servicecatalogappregistry): Allow user to control stack id via stack name for Application stack Feb 15, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 8181c8f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 0c7c7e4 into aws:main Feb 16, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@jungle-amazon jungle-amazon deleted the stack_id_app_associator branch February 20, 2023 19:18
mergify bot pushed a commit that referenced this pull request Mar 2, 2023
…ter deployment (#24409)

Background:
* Customers wish to control or modify the stack id of the Application stack to follow their project conventions within CDK.
* In previous [fix](#24171), we had deprecated the stack id to push users towards using stack name as the main mechanism for stack identification.
* Default stack name as per this [fix](#24171) has `Application Name` embedded in it. If customers wants to **update the application name**, then a new stack name as well as new stack id will be generated, which will eventually create a new Application in a new stack.

Problem:
* Application associator associates all stacks in scope to an application.
* With `ApplicationName` in stack name, we will create an application within a stack. When customers attempt to update the name of the application, we will create a new stack and eventually create a new application, which is the first problem.
* In this new application within the new stack, we will try to associate all stacks within the `app` scope, which is already associated to the original application in the original stack and hence cannot be further associated to the new application which will make the deployment of the stack with updated name fail.

Fix:
* We will continue to honor customer provided stack Name.
* If no stack name is provided, then we will default it to `ApplicationAssociator-${hashValues(scope.node.addr)}-Stack`, which doesnt contain `Application Name`.

Related Links:
* Previous PR which introduced this bug: #24171

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
…ter deployment (aws#24409)

Background:
* Customers wish to control or modify the stack id of the Application stack to follow their project conventions within CDK.
* In previous [fix](aws#24171), we had deprecated the stack id to push users towards using stack name as the main mechanism for stack identification.
* Default stack name as per this [fix](aws#24171) has `Application Name` embedded in it. If customers wants to **update the application name**, then a new stack name as well as new stack id will be generated, which will eventually create a new Application in a new stack.

Problem:
* Application associator associates all stacks in scope to an application.
* With `ApplicationName` in stack name, we will create an application within a stack. When customers attempt to update the name of the application, we will create a new stack and eventually create a new application, which is the first problem.
* In this new application within the new stack, we will try to associate all stacks within the `app` scope, which is already associated to the original application in the original stack and hence cannot be further associated to the new application which will make the deployment of the stack with updated name fail.

Fix:
* We will continue to honor customer provided stack Name.
* If no stack name is provided, then we will default it to `ApplicationAssociator-${hashValues(scope.node.addr)}-Stack`, which doesnt contain `Application Name`.

Related Links:
* Previous PR which introduced this bug: aws#24171

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

servicecatalogappregistry: Allow users to set ApplicationAssociator stack ID
3 participants