-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(servicecatalogappregistry): stackId construct prop for ApplicationAssociator #22508
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -28,6 +28,14 @@ export interface ApplicationAssociatorProps { | |||||
*/ | ||||||
readonly description?: string; | ||||||
|
||||||
/** | ||||||
* Stack ID. | ||||||
* | ||||||
* @default - ApplicationAssociatorStack | ||||||
* | ||||||
*/ | ||||||
readonly stackId?: string; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think that just stackId here may be confusing. Let's make the field name more specific. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we also rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
||||||
/** | ||||||
* Stack properties. | ||||||
* | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the additional context here. |
||||||
*/ | ||||||
export class ApplicationAssociator extends Construct { | ||||||
/** | ||||||
|
@@ -56,7 +66,7 @@ export class ApplicationAssociator extends Construct { | |||||
constructor(scope: cdk.App, id: string, props: ApplicationAssociatorProps) { | ||||||
super(scope, id); | ||||||
|
||||||
const applicationStack = new cdk.Stack(scope, 'ApplicationAssociatorStack', props.stackProps); | ||||||
const applicationStack = new cdk.Stack(scope, props.stackId ?? 'ApplicationAssociatorStack', props.stackProps); | ||||||
|
||||||
if (!!props.applicationArnValue) { | ||||||
this.application = Application.fromApplicationArn(applicationStack, 'ImportedApplication', props.applicationArnValue); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
stackProps: { | ||
stackName: 'AppRegistryApplicationAssociatorStack', | ||
}, | ||
|
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.
Can we not use stackName for the stackId instead of adding another prop? This seems redundant.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheRealAmazonKendra if we use
stackName
for stack ID, does it require a dedicated test?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.