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

Jump to bottom

feat(servicecatalogappregistry): stackId construct prop for ApplicationAssociator #22508

Closed
wants to merge 2 commits into from
Closed

feat(servicecatalogappregistry): stackId construct prop for ApplicationAssociator #22508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccca4c2
Add stackId to ApplicationAssociator construct props
alexpulver Oct 14, 2022
6e389ed
Merge branch 'main' into appregistry-stack-id
alexpulver Oct 22, 2022
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
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplicati
});
```

You can also specify `stackId` to override the default value:

```ts
const app = new App();
const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', {
applicationName: 'MyAssociatedApplication',
stackId: 'MyAssociatedApplicationStack',
stackProps: {
stackName: 'MyAssociatedApplicationStack',
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor

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.

env: {account: '123456789012', region: 'us-east-1'},
},
});
```

If you want to re-use an existing Application with ARN: `arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId`
and want to associate all stacks in the `App` scope to your imported application, then use as shown in the example below:

Expand Down
12 changes: 11 additions & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/lib/application-associator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ export interface ApplicationAssociatorProps {
*/
readonly description?: string;

/**
* Stack ID.
*
* @default - ApplicationAssociatorStack
*
*/
readonly stackId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
readonly stackId?: string;
readonly applicationStackId?: string;

I think that just stackId here may be confusing. Let's make the field name more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we also rename stackProps to applicationStackProps then?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
*
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the additional context here.

*/
export class ApplicationAssociator extends Construct {
/**
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion ...ot/ApplicationAssociatorStack.assets.json → ...tryApplicationAssociatorStack.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files": {
"4285054f947789e255e76c75b889b3b216adabb0b3f990c8966c18459cdf7b35": {
"source": {
"path": "ApplicationAssociatorStack.template.json",
"path": "AppRegistryApplicationAssociatorStack.template.json",
"packaging": "file"
},
"destinations": {
Expand Down
0 .../ApplicationAssociatorStack.template.json → ...yApplicationAssociatorStack.template.json
View file
Open in desktop
File renamed without changes.
31 changes: 15 additions & 16 deletions ...pregistry/test/application-associator.all-stacks-association.integ.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
}
},
"dependencies": [
"ApplicationAssociatorStack",
"AppRegistryApplicationAssociatorStack",
"integservicecatalogappregistryapplicationresourcesStack4399A149.assets"
],
"metadata": {
Expand Down Expand Up @@ -100,7 +100,7 @@
}
},
"dependencies": [
"ApplicationAssociatorStack",
"AppRegistryApplicationAssociatorStack",
"integ-servicecatalogappregistry-application.assets"
],
"metadata": {
Expand Down Expand Up @@ -135,40 +135,39 @@
},
"displayName": "integ-servicecatalogappregistry-application"
},
"ApplicationAssociatorStack.assets": {
"AppRegistryApplicationAssociatorStack.assets": {
"type": "cdk:asset-manifest",
"properties": {
"file": "ApplicationAssociatorStack.assets.json",
"file": "AppRegistryApplicationAssociatorStack.assets.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version"
}
},
"ApplicationAssociatorStack": {
"AppRegistryApplicationAssociatorStack": {
"type": "aws:cloudformation:stack",
"environment": "aws://unknown-account/unknown-region",
"properties": {
"templateFile": "ApplicationAssociatorStack.template.json",
"templateFile": "AppRegistryApplicationAssociatorStack.template.json",
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/4285054f947789e255e76c75b889b3b216adabb0b3f990c8966c18459cdf7b35.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
"ApplicationAssociatorStack.assets"
"AppRegistryApplicationAssociatorStack.assets"
],
"lookupRole": {
"arn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-lookup-role-${AWS::AccountId}-${AWS::Region}",
"requiresBootstrapStackVersion": 8,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version"
},
"stackName": "AppRegistryApplicationAssociatorStack"
}
},
"dependencies": [
"ApplicationAssociatorStack.assets"
"AppRegistryApplicationAssociatorStack.assets"
],
"metadata": {
"/ApplicationAssociatorStack": [
"/AppRegistryApplicationAssociatorStack": [
{
"type": "aws:cdk:warning",
"data": "Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack."
Expand All @@ -178,32 +177,32 @@
"data": "Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack."
}
],
"/ApplicationAssociatorStack/DefaultCdkApplication/Resource": [
"/AppRegistryApplicationAssociatorStack/DefaultCdkApplication/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "DefaultCdkApplication4573D5A3"
}
],
"/ApplicationAssociatorStack/AppRegistryAssociation": [
"/AppRegistryApplicationAssociatorStack/AppRegistryAssociation": [
{
"type": "aws:cdk:logicalId",
"data": "AppRegistryAssociation"
}
],
"/ApplicationAssociatorStack/BootstrapVersion": [
"/AppRegistryApplicationAssociatorStack/BootstrapVersion": [
{
"type": "aws:cdk:logicalId",
"data": "BootstrapVersion"
}
],
"/ApplicationAssociatorStack/CheckBootstrapVersion": [
"/AppRegistryApplicationAssociatorStack/CheckBootstrapVersion": [
{
"type": "aws:cdk:logicalId",
"data": "CheckBootstrapVersion"
}
]
},
"displayName": "ApplicationAssociatorStack"
"displayName": "AppRegistryApplicationAssociatorStack"
}
}
}
14 changes: 7 additions & 7 deletions ...ogappregistry/test/application-associator.all-stacks-association.integ.snapshot/tree.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"path": "Tree",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.95"
"version": "10.1.129"
}
},
"integ-servicecatalogappregistry-application": {
Expand Down Expand Up @@ -76,17 +76,17 @@
"version": "0.0.0"
}
},
"ApplicationAssociatorStack": {
"id": "ApplicationAssociatorStack",
"path": "ApplicationAssociatorStack",
"AppRegistryApplicationAssociatorStack": {
"id": "AppRegistryApplicationAssociatorStack",
"path": "AppRegistryApplicationAssociatorStack",
"children": {
"DefaultCdkApplication": {
"id": "DefaultCdkApplication",
"path": "ApplicationAssociatorStack/DefaultCdkApplication",
"path": "AppRegistryApplicationAssociatorStack/DefaultCdkApplication",
"children": {
"Resource": {
"id": "Resource",
"path": "ApplicationAssociatorStack/DefaultCdkApplication/Resource",
"path": "AppRegistryApplicationAssociatorStack/DefaultCdkApplication/Resource",
"attributes": {
"aws:cdk:cloudformation:type": "AWS::ServiceCatalogAppRegistry::Application",
"aws:cdk:cloudformation:props": {
Expand All @@ -107,7 +107,7 @@
},
"AppRegistryAssociation": {
"id": "AppRegistryAssociation",
"path": "ApplicationAssociatorStack/AppRegistryAssociation",
"path": "AppRegistryApplicationAssociatorStack/AppRegistryAssociation",
"attributes": {
"aws:cdk:cloudformation:type": "AWS::ServiceCatalogAppRegistry::ResourceAssociation",
"aws:cdk:cloudformation:props": {
Expand Down
1 change: 1 addition & 0 deletions ...aws-servicecatalogappregistry/test/integ.application-associator.all-stacks-association.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The 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',
},
Expand Down