feat(servicecatalogappregistry): stackId construct prop for ApplicationAssociator #22508
Conversation
|
|
github-actions
bot
added
p2
repeat-contributor
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
| * Stack ID. | ||
| * | ||
| */ | ||
| readonly stackId: string; |
There was a problem hiding this comment.
Can we make this optional instead? In case customers do not want to provide stackId, then we can instead default it to ApplicationAssociator
There was a problem hiding this comment.
Thank you for the suggestion! It also makes the change incremental and not breaking, which is great 😃. I pushed the update, although used the previous ApplicationAssociatorStack value to avoid introducing a breaking change.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
| * @default - ApplicationAssociatorStack | ||
| * | ||
| */ | ||
| readonly stackId?: string; |
There was a problem hiding this comment.
| readonly stackId?: string; | |
| readonly applicationStackId?: string; |
I think that just stackId here may be confusing. Let's make the field name more specific.
There was a problem hiding this comment.
Shouldn't we also rename stackProps to applicationStackProps then?
There was a problem hiding this comment.
I think so? Given your role, I think you would have a better idea of the user experience here so I'm wondering what your thoughts are on this change. I prefer it for clarity but I don't use this service.
| @@ -10,6 +10,7 @@ const stack = new cdk.Stack(app, 'integ-servicecatalogappregistry-application'); | |||
| new appreg.ApplicationAssociator(app, 'RegisterCdkApplication', { | |||
| applicationName: 'AppRegistryAssociatedApplication', | |||
| description: 'Testing AppRegistry ApplicationAssociator', | |||
| stackId: 'AppRegistryApplicationAssociatorStack', | |||
There was a problem hiding this comment.
Instead of just adding this field here, please convert this to the new integ test type and add a second stack with the applicationStackId supplied. The new style of tests allows for multiple test cases.
| applicationName: 'MyAssociatedApplication', | ||
| stackId: 'MyAssociatedApplicationStack', | ||
| stackProps: { | ||
| stackName: 'MyAssociatedApplicationStack', |
There was a problem hiding this comment.
Can we not use stackName for the stackId instead of adding another prop? This seems redundant.
There was a problem hiding this comment.
We definitely can, although it will cause stack replacement for existing customers. Given that this is alpha, would you be okay that I'll make that change?
There was a problem hiding this comment.
@TheRealAmazonKendra if we use stackName for stack ID, does it require a dedicated test?
There was a problem hiding this comment.
Breaking changes are fine, just make sure that the fact that it's breaking shows up in the title. From your perspective, would using the stackName satisfy your use case and also not require extra fields that the user has to care about?
There was a problem hiding this comment.
And I'd say that as long as the change shows up in integration tests and impacts the outputs in a way we see when you run the tests, I don't think any additional ones should be needed. But, I'd ask that you change the name of the stack in the test below so we can see the impact.
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * @default - ApplicationAssociatorStack | ||
| * | ||
| */ | ||
| readonly stackId?: string; |
There was a problem hiding this comment.
I think so? Given your role, I think you would have a better idea of the user experience here so I'm wondering what your thoughts are on this change. I prefer it for clarity but I don't use this service.
| applicationName: 'MyAssociatedApplication', | ||
| stackId: 'MyAssociatedApplicationStack', | ||
| stackProps: { | ||
| stackName: 'MyAssociatedApplicationStack', |
There was a problem hiding this comment.
Breaking changes are fine, just make sure that the fact that it's breaking shows up in the title. From your perspective, would using the stackName satisfy your use case and also not require extra fields that the user has to care about?
| applicationName: 'MyAssociatedApplication', | ||
| stackId: 'MyAssociatedApplicationStack', | ||
| stackProps: { | ||
| stackName: 'MyAssociatedApplicationStack', |
There was a problem hiding this comment.
And I'd say that as long as the change shows up in integration tests and impacts the outputs in a way we see when you run the tests, I don't think any additional ones should be needed. But, I'd ask that you change the name of the stack in the test below so we can see the impact.
| @@ -45,6 +53,8 @@ export interface ApplicationAssociatorProps { | |||
| * | |||
| * If cross account stack is detected, then this construct will automatically share the application to consumer accounts. | |||
| * Cross account feature will only work for non environment agnostic stacks. | |||
| * | |||
| * The construct creates a dedicated stack for the application. | |||
There was a problem hiding this comment.
Thanks for adding the additional context here.
|
Hi @santanugho, should I close this PR in favor of #22644? |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
5 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
Superceded by #22644 |
…ack name for Application stack (#24171) * 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](#23823), 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](https://github.com/aws/aws-cdk/releases/tag/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: ```log 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: ```typescript 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: * Previous PR: #23823 * Previous GitHub issue: #23861 * Original reason that introduced `stackId`: #22508 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add optional
stackIdconstruct prop toApplicationAssociatorclass and clarify documentation.Currently, I can't control the stack ID that
ApplicationAssociatorclass defines:aws-cdk/packages/@aws-cdk/aws-servicecatalogappregistry/lib/application-associator.ts
Line 59 in 5a0595e
I maintain a certain convention in my projects for the stack ID. For example:
<Workload><Component><Suffix>(e.g.UserManagement*Backend*Sandbox). I wanted the stack ID thatApplicationAssociatorclass defines to align with that convention (e.g.UserManagement*Backend*Application). This way, I control and understand the origin of stack IDs listed incdk synthoutput.With current beahvior, the
cdk synthoutput I get is:The
ApplicationAssociatorStackname was surprising for me, as I didn't specify it. I had to look atApplicationAssociatorsource code to understand where it's coming from. It also wasn't obvious from the documentation thatApplicationAssociatorcreates a dedicated stack for the application. I added this clarification.All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license