fix(servicecatalogappregistry): Associate an application with attribute group #24378

santanugho · 2023-02-28T18:46:15Z

Application-AttributeGroup association happens in ApplicationStack.
Therefore before the deployment of ApplicationStack, AttributeGroup stack should have been deployed.
But with ApplicationAssociator where we associate all the stacks with the application (created as a part of ApplicationAssociator), attributeGroup stack now depends upon ApplicationAssociator stack to be created (since ResourceAssociation happens inside ResourceStack).
This creates a circular dependency and hence cdk synth fails. We found this bug during internal testing
This PR address this circular dependency issue, by allowing the customers of ApplicationAssociator to associate an attribute group in attribute group stack.

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

TheRealAmazonKendra

Thank you for your contribution, we cannot provide a meaningful review until there is a passing build. If you have questions about the build failure, please let us know.

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests). Specifically, a fix should state the bug, not the solution.

Lastly, is there an open issue for this? If so, please link it in the PR body.

Pull request has been modified.

TheRealAmazonKendra · 2023-03-01T01:44:04Z

Thanks for getting the build passing. The last two pieces of my comment will still need to be addressed.

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests). Specifically, a fix should state the bug, not the solution.

Lastly, is there an open issue for this? If so, please link it in the PR body.

santanugho · 2023-03-01T02:30:57Z

Thanks for getting the build passing. The last two pieces of my comment will still need to be addressed.

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests). Specifically, a fix should state the bug, not the solution.

Lastly, is there an open issue for this? If so, please link it in the PR body.

PR title confirms to the conventional commit standard. This PR is for a bug fix in servicecatalogappregistry and the PR title is as per that standard.
This bug has been reported internally and there is no github issue open for it.

rix0rrr · 2023-03-01T09:14:15Z

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

+class CustomAppRegistryAttributeGroup extends cdk.Stack {
+  public readonly attributeGroup: appreg.AttributeGroup
+  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
+    super(scope, id, props);
+    const myAttributeGroup = new appreg.AttributeGroup(app, 'MyFirstAttributeGroup', {
+      attributeGroupName: 'MyAttributeGroupName',
+      description: 'Test attribute group',
+      attributes: {},
+    });
+
+    this.attributeGroup = myAttributeGroup;
+  }
+}


Do we really expect people to define a stack for their AttributeGroup? Couldn't it go into one of the other stacks?

For example, wouldn't it make sense for a user to do:

const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', { applications: [ /* ... */], attributes: { // ... }, });

attributes is not a valid input as per ApplicationAssociatorProps. Attribute group can definitely be a part of some other stack, however customers need to explicitly associate the attribute group to the application, as ApplicationAssociator is responsible to associate the stacks within the scope, it wont automatically associate the underlying AGs within a stack.

attributes is not a valid input as per ApplicationAssociatorProps

I understand.

The nice thing about software is that it is not like the laws of physics. It is in fact changeable.

FACT: "attributes" can not be passed to the ApplicationAssociator

QUESTION: would it be better/easier/more convenient/more obvious IF "attributes" could be passed to ApplicationAssociator, and the creation of the AttributeGroup happens implicitly?

TBH I like this idea of including attribute-group in application-associator construct. However, can we first prioritize this bug fix which will unblock our customers and they should be able to associate an attribute group to an application.

I will do some more deep dive on including attribute-group in application-associator, its pros and cons and will let you know about our plan to modify application-associator in order to include attribute-group.

Let me know if this plan works for you.

rix0rrr · 2023-03-01T09:17:53Z

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

+declare const application: appreg.Application;
+declare const attributeGroup: appreg.AttributeGroup;
+attributeGroup.associateApplicationWithAttributeGroup(application);


This is good, although the method name is a little long. Given that the subject is of type AttributeGroup and the object is of type Application, do you really think we need the words "Application" and "AttributeGroup" in the method name?

Why not

attributeGroup.associate(application); // or attributeGroup.associateWith(application); // or attributeGroup.associateTo(application);

Yeah associateWith makes a lot more sense. Changed the same in the latest version.

rix0rrr

See my previous comment: do we want to force the user to create an attributegroup themselves?

I think users would prefer to minimize the amount of stacks in their account if possible.

Pull request has been modified.

mergify · 2023-03-06T09:14:14Z

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 · 2023-03-06T09:55:08Z

AWS CodeBuild CI Report

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

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

mergify · 2023-03-06T10:00:49Z

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

…te group (aws#24378) `Application-AttributeGroup` association happens in `ApplicationStack`. Therefore before the deployment of `ApplicationStack`, `AttributeGroup` stack should have been deployed. But with `ApplicationAssociator `where we associate all the stacks with the application (created as a part of `ApplicationAssociator`), attributeGroup stack now depends upon `ApplicationAssociator` stack to be created (since ResourceAssociation happens inside ResourceStack). This creates a circular dependency and hence cdk synth fails. We found this bug during internal testing This PR address this circular dependency issue, by allowing the customers of `ApplicationAssociator` to associate an attribute group in attribute group stack. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

sangosh and others added 6 commits January 11, 2023 13:21

Issue-23641: Fixing dynamic description bug

3bbc9a1

Changing the integration test

4262fdb

Merge branch 'main' into main

6208b23

Merge branch 'aws:main' into main

fe90d28

Merge branch 'aws:main' into main

f682b68

Associate application with attribute group

ae03cb5

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK p2 labels Feb 28, 2023

aws-cdk-automation requested a review from a team February 28, 2023 18:46

TheRealAmazonKendra previously requested changes Feb 28, 2023

View reviewed changes

Associate application to AG

c3dd40a

rix0rrr reviewed Mar 1, 2023

View reviewed changes

Associate application to attribute group

fe247c8

rix0rrr previously requested changes Mar 2, 2023

View reviewed changes

Merge branch 'main' into Issue-AWS242-27542

9249be2

Fixing the test

7a2804c

rix0rrr approved these changes Mar 6, 2023

View reviewed changes

Merge branch 'main' into Issue-AWS242-27542

a329869

mergify bot merged commit d1264c1 into aws:main Mar 6, 2023

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

fix(servicecatalogappregistry): Associate an application with attribute group #24378

fix(servicecatalogappregistry): Associate an application with attribute group #24378

santanugho commented Feb 28, 2023

TheRealAmazonKendra left a comment

TheRealAmazonKendra commented Mar 1, 2023

santanugho commented Mar 1, 2023

rix0rrr Mar 1, 2023

santanugho Mar 1, 2023

rix0rrr Mar 2, 2023 •

edited

Loading

santanugho Mar 2, 2023

rix0rrr Mar 1, 2023

santanugho Mar 1, 2023

rix0rrr left a comment

mergify bot commented Mar 6, 2023

aws-cdk-automation commented Mar 6, 2023

mergify bot commented Mar 6, 2023

fix(servicecatalogappregistry): Associate an application with attribute group #24378

fix(servicecatalogappregistry): Associate an application with attribute group #24378

Conversation

santanugho commented Feb 28, 2023

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Mar 1, 2023

santanugho commented Mar 1, 2023

rix0rrr Mar 1, 2023

Choose a reason for hiding this comment

santanugho Mar 1, 2023

Choose a reason for hiding this comment

rix0rrr Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

santanugho Mar 2, 2023

Choose a reason for hiding this comment

rix0rrr Mar 1, 2023

Choose a reason for hiding this comment

santanugho Mar 1, 2023

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 6, 2023

aws-cdk-automation commented Mar 6, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 6, 2023

rix0rrr Mar 2, 2023 •

edited

Loading