feat(servicecatalogappregistry): add attribute groups to an application #24672

liwewang-amazon · 2023-03-17T18:26:34Z

To associate a attribute group to an application created in ApplicationAssociator, customers have to use AttributeGroup construct separately to create and associate the attribute group separately. This makes the AttributeGroup and AttributeGroupAssociation created in another stack than ApplicationAssociator stack.
This commits provides an one-stop action, i.e. Application.addAttributeGroup(), to create and associate attribute group on Application Construct. This solution not only makes attribute group creation and association easier for customer who uses Application construct, but also lets customer to have attribute groups and attribute group associations for the ApplicationAssociator applications in the same stack.

Application.addAttributeGroup() has id in the parameters, for two reasons:

consistent with the experience where customer can define logical ID when using new AttributeGroup()
complexity of deciding logical ID from the attribute group input:
- We have to make sure update attributes/description won't trigger create and then delete but update, which will cause name conflict exception.
- We also don't want to generate logical ID from attribute group name only, as if two Application.addAttributeGroup() method calls with the same name will result in construct ID conflict. This exposes implementation details and makes it hard to customers to debug and resolve.

BREAKING CHANGE: This commit contains destructive changes to the RAM Share.
Since the application RAM share name is calculated by the application construct, where one method is added. Integration test detects a breaking change where RAM share will be created. Integration test snapshot is updated to cater this destructive change.

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

aws-cdk-automation

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.

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

packages/@aws-cdk/aws-servicecatalogappregistry/README.md

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

rix0rrr · 2023-03-20T10:28:20Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+  /**
+   * Enforces a particular physical attribute group name.
+   */
+  readonly attributeGroupName: string;


Why is this not optional?

Attribute group name is the key to calculate attribute group construct id to make sure that multiple addAttributeGroup calls won't generate the same construct ID. Since name is unique, if addAttributeGroup is called twice with the same attributeGroupName, construct duplicate error will be thrown. Both Names.uniqueNodeId() and Names.uniqueResourceName() generate id from the path of the construct, i.e. path of the application. As a result, two addAttributeGroup calls with different names\attributes\descriptions generate two AttributeGroup constructs with the same construct ID.

This should be required even if we use customer provided construct id for attribute group.

Attribute group name is the key human readable identifier our service offers for customer to find the attribute group, we want to make it explicit, so that it will can make use of the name to look for the attribute group in other platforms like console or SDK client.

You are aware that 99% of other constructs allow the user to not specify a name and have the system one generate for them?

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

rix0rrr · 2023-03-20T10:29:30Z

BREAKING CHANGE: This commit contains destructive changes to the RAM Share.
Since the application RAM share name is calculated by the application construct, where one method is added.

I don't fully understand this. What is the actual user impact (will it block deployments, interrupt cross-account access for a couple of minutes, what?), and do you think the resource replacement is reasonable behavior?

liwewang-amazon · 2023-03-20T18:58:51Z

I don't fully understand this. What is the actual user impact (will it block deployments, interrupt cross-account access for a couple of minutes, what?), and do you think the resource replacement is reasonable behavior?

I agree that resource replacement is not reasonable behavior, I would like to resolve this as a followup. The issue we are face is the same as addAttributeGroup: we want to call the method shareApplication multiple times and create constructs with different construct IDs.

Pull request has been modified.

…group to the application. <body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Change Attribute Group construct ID generation from attribute group name. Change ApplicationAssociator application to a getter. Remove logical ID from addAttributeGroup props. <body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

…nstruct id. <body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

aws-cdk-automation · 2023-03-22T04:27:38Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 07c0a1e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2023-03-22T08:52:27Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+  /**
+   * Enforces a particular physical attribute group name.
+   */
+  readonly attributeGroupName: string;


You are aware that 99% of other constructs allow the user to not specify a name and have the system one generate for them?

rix0rrr · 2023-03-22T08:53:00Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+      attributes: props.attributes,
+      description: props.description,
+    });
+    new CfnAttributeGroupAssociation(this, `AttributeGroupAssociation${this.generateUniqueHash(attributeGroup.node.addr)}`, {


node.addr is already a hash :)

mergify · 2023-03-22T09:36:43Z

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

… to Application (#24760) `Application.shareApplication` can be called multiple times which generates different resources within the same `Application` construct. The share construct id generation was based on child length of `Application` construct. This makes every change to the `Application` construct can unnecessarily replace the RAM share when the share actually needs no update. Similar to the solution for `addAttributeGroup` introduced in the [previous update](#24672), this commit starts to require both construct id and share name in the `Application.shareApplication` method input, which are used to create RAM share construct. For `ApplicationAssociator`, `Application.shareApplication` is called for cross-account stack associations. Share construct id depends on the node unique id of the application in `ApplicationAssociator` and the node unique id of the stack to be associated. This reduces unnecessary share replacement in `ApplicationAssociator` stack. BREAKING CHANGE: This commit involves share replacement during the deployment of `ApplicationAssociator` due to share construct id update. After this change, frequent share replacements due to structural change in `Application` construct should be avoided. `Application.shareApplication` starts to require construct id (first argument) and share name (added in `ShareOption`) as input. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…on (aws#24672) To associate a attribute group to an application created in `ApplicationAssociator`, customers have to use `AttributeGroup` construct separately to create and associate the attribute group separately. This makes the `AttributeGroup` and `AttributeGroupAssociation` created in another stack than `ApplicationAssociator` stack. This commits provides an one-stop action, i.e. `Application.addAttributeGroup()`, to create and associate attribute group on `Application` Construct. This solution not only makes attribute group creation and association easier for customer who uses `Application` construct, but also lets customer to have attribute groups and attribute group associations for the `ApplicationAssociator` applications in the same stack. `Application.addAttributeGroup()` has `id` in the parameters, for two reasons: - consistent with the experience where customer can define logical ID when using `new AttributeGroup()` - complexity of deciding logical ID from the attribute group input: - We have to make sure update attributes/description won't trigger create and then delete but update, which will cause name conflict exception. - We also don't want to generate logical ID from attribute group name only, as if two `Application.addAttributeGroup()` method calls with the same name will result in construct ID conflict. This exposes implementation details and makes it hard to customers to debug and resolve. BREAKING CHANGE: This commit contains destructive changes to the RAM Share. Since the application RAM share name is calculated by the application construct, where one method is added. Integration test detects a breaking change where RAM share will be created. Integration test snapshot is updated to cater this destructive change. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… to Application (aws#24760) `Application.shareApplication` can be called multiple times which generates different resources within the same `Application` construct. The share construct id generation was based on child length of `Application` construct. This makes every change to the `Application` construct can unnecessarily replace the RAM share when the share actually needs no update. Similar to the solution for `addAttributeGroup` introduced in the [previous update](aws#24672), this commit starts to require both construct id and share name in the `Application.shareApplication` method input, which are used to create RAM share construct. For `ApplicationAssociator`, `Application.shareApplication` is called for cross-account stack associations. Share construct id depends on the node unique id of the application in `ApplicationAssociator` and the node unique id of the stack to be associated. This reduces unnecessary share replacement in `ApplicationAssociator` stack. BREAKING CHANGE: This commit involves share replacement during the deployment of `ApplicationAssociator` due to share construct id update. After this change, frequent share replacements due to structural change in `Application` construct should be avoided. `Application.shareApplication` starts to require construct id (first argument) and share name (added in `ShareOption`) as input. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Mar 17, 2023

aws-cdk-automation requested a review from a team March 17, 2023 18:26

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Mar 17, 2023

aws-cdk-automation previously requested changes Mar 17, 2023

View reviewed changes

liwewang-amazon changed the title ~~feat(servicecatalogappegistry) support one-stop action to create and associate attribute group for an application~~ feat(servicecatalogappegistry) support one stop action to create and associate attribute group for an application Mar 17, 2023

liwewang-amazon changed the title ~~feat(servicecatalogappegistry) support one stop action to create and associate attribute group for an application~~ feat(servicecatalogappegistry): support one stop action to create and associate attribute group for an application Mar 17, 2023

liwewang-amazon changed the title ~~feat(servicecatalogappegistry): support one stop action to create and associate attribute group for an application~~ feat(servicecatalogappegistry): support one-stop action to create and associate attribute group for an application Mar 17, 2023

liwewang-amazon changed the title ~~feat(servicecatalogappegistry): support one-stop action to create and associate attribute group for an application~~ feat(servicecatalogappregistry): support one-stop action to create and associate attribute group for an application Mar 17, 2023

liwewang-amazon marked this pull request as ready for review March 17, 2023 20:35

rix0rrr previously requested changes Mar 20, 2023

View reviewed changes

liwewang-amazon marked this pull request as draft March 21, 2023 22:18

liwewang-amazon force-pushed the associate_new_ag branch from f91fd5e to 2167bf5 Compare March 21, 2023 22:48

liwewang-amazon added 9 commits March 21, 2023 20:47

Add associateNewAttributeGroup to create and associate the attribute …

570c77e

…group to the application. <body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Support create & associate attribute group in Application construct

ddd8fab

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Change addAG param name

aa97fd7

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

fix readme

540af2e

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Add id into addAttributeGroup input to provide to AttributeGroup co…

1dafe29

…nstruct id. <body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Change doc for addAttributeGroup method.

f21f11b

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

Make attribute group name as required.

d4753c6

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

n to N

07c0a1e

<body> Testing done ------------------------------------- * Related items ------------------------------------ * SIM/auto-cut ticket

liwewang-amazon force-pushed the associate_new_ag branch from 2167bf5 to 07c0a1e Compare March 22, 2023 03:47

liwewang-amazon marked this pull request as ready for review March 22, 2023 04:37

rix0rrr changed the title ~~feat(servicecatalogappregistry): support one-stop action to create and associate attribute group for an application~~ feat(servicecatalogappregistry): add attribute groups to an application Mar 22, 2023

rix0rrr approved these changes Mar 22, 2023

View reviewed changes

mergify bot merged commit 7baffa2 into aws:main Mar 22, 2023

liwewang-amazon deleted the associate_new_ag branch March 22, 2023 22:31

liwewang-amazon mentioned this pull request Mar 23, 2023

fix(servicecatalogappregistry): RAM Share is replaced on every change to Application #24760

Merged

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalogappregistry): add attribute groups to an application #24672

feat(servicecatalogappregistry): add attribute groups to an application #24672

liwewang-amazon commented Mar 17, 2023 •

edited

Loading

aws-cdk-automation left a comment

rix0rrr Mar 20, 2023

liwewang-amazon Mar 20, 2023

liwewang-amazon Mar 21, 2023

rix0rrr Mar 22, 2023

rix0rrr commented Mar 20, 2023 •

edited

Loading

liwewang-amazon commented Mar 20, 2023

aws-cdk-automation commented Mar 22, 2023

rix0rrr Mar 22, 2023

rix0rrr Mar 22, 2023

mergify bot commented Mar 22, 2023

feat(servicecatalogappregistry): add attribute groups to an application #24672

feat(servicecatalogappregistry): add attribute groups to an application #24672

Conversation

liwewang-amazon commented Mar 17, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rix0rrr Mar 20, 2023

Choose a reason for hiding this comment

liwewang-amazon Mar 20, 2023

Choose a reason for hiding this comment

liwewang-amazon Mar 21, 2023

Choose a reason for hiding this comment

rix0rrr Mar 22, 2023

Choose a reason for hiding this comment

rix0rrr commented Mar 20, 2023 • edited Loading

liwewang-amazon commented Mar 20, 2023

aws-cdk-automation commented Mar 22, 2023

AWS CodeBuild CI Report

rix0rrr Mar 22, 2023

Choose a reason for hiding this comment

rix0rrr Mar 22, 2023

Choose a reason for hiding this comment

mergify bot commented Mar 22, 2023

liwewang-amazon commented Mar 17, 2023 •

edited

Loading

rix0rrr commented Mar 20, 2023 •

edited

Loading