feat(servicecatalog): Create TagOptions Construct #18314

arcrank · 2022-01-07T18:42:55Z

Previously TagOptions were defined via an interface and we only created the underlying
resources upon an association. This broke CX if tagoptions were mangaged centrally. We move to
make the TagOptions class a wrapper around aggregate individual TagOptions.

BREAKING CHANGE: TagOptions now have scope and props argument in constructor, and data is now passed via a allowedValueForTags field in props

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

Previously TagOptions were defined via an interface and we only created the underlying resources upon an association. This broke CX if tagoptions were mangaged centrally. We move to make the TagOptions class a wrapper around aggregate individual TagOptions. Based on updated contributing guidelines, need to figure out how to note this as a breaking change, or figure out how to properly deprecate old version.

gitpod-io · 2022-01-07T18:42:58Z

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

arcrank · 2022-01-10T17:16:37Z

This should address: #17753

skinny85

First round of comments. We definitely need to minimize the amount of changes we are making to the tests in this PR, as we're changing production code at the same time. I also thing TagOptions should become a full Construct.

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

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

packages/@aws-cdk/aws-servicecatalog/test/integ.tag-option.ts

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

packages/@aws-cdk/aws-servicecatalog/test/tag-option.test.ts

skinny85 · 2022-01-12T01:55:12Z

This should address: #17753

Let's add a Fixes #17753 to the PR description then.

Pull request has been modified.

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

arcrank · 2022-01-14T04:02:49Z

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

- Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOption', 3); //Generates a resource for each unique key-value pair
- Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOptionAssociation', 3);
- }),
-
 test('fails to add tag options with invalid minimum key length', () => {


These are also not really valid but we can modify it slightly to still work, unfortunately still a lot of git churn. Moving the creation away from the association means that early failures happen on creation not via association calls.

OK. If these now fail on creation, let's modify the test and its description to reflect that (being careful to minimize the changes still).

I understand this will mean these tests are now in the wrong file, but we'll live with that for now.

updated with modified wording and minimized changes.

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

arcrank · 2022-01-14T04:12:37Z

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

+ */
+ public readonly _tagOptionsMap: { [key: string]: CfnTagOption };
+
+ constructor(scope: Construct, id: string, tagOptions: {[key: string]: string[] }) {


The cdk linting doesn't like this setup (no interface and no props struct) but I don't particularly see value of having classes that are just pass throughs. I don't know if making it props with a tagOptions field is better since it seems redundant to do

const tagO = new servicecatalog.TagOptions(this, 'TagOptions', { tagOptions : { key1: ['v1', 'v2', 'v3'], } });

Only if its a little bit of future proofing in case we want to add another field to props then it would be easier to make change.
I will defer to you though on this.

I think linting rules have become more stringent? Build is failing since you apparently need to disable interface ref in js docs ( I had hoped I could just do it in package.json).

Let's just introduce a TagOptionsProps interface as the third argument, like you suggested in #18314 (comment).

that works, leaves more wiggle room to not break things in future should changes happen and we want to modify props.

skinny85

We are getting closer, but I think this needs a couple more iterations before we can call it "Done".

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

skinny85 · 2022-01-18T23:11:53Z

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

- * List of CfnTagOption
- */
- public readonly tagOptionsMap: { [key: string]: string[] };
+ * TagOption keys and associated list of possible values.


I feel like these docs need some love. How about something like:

Suggested change

* TagOption keys and associated list of possible values.

* The values that are allowed to be set for specific tags.

* The keys of the map are the names of the tags,

* and the values of the map are a list of allowed values of that particular tag.

Feel free to add more as you see fit.

It can be confusing with keys in a map struct and keys to a tag, but I think we should stick with using tag key as the wording. I don't know if I've seen tags referenced with a name and not just key-value.

Fine, then use the name "tag key". That's why I said "something like that" and "feel free to add more as you see fit" 🙂.

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

skinny85 · 2022-01-18T23:26:05Z

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

+
+ constructor(scope: Construct, id: string, props: TagOptionsProps) {
+ super(scope, id);
+ this._tagOptionsMap = this.createUnderlyingTagOptions(props.tagOptions);


Is it valid to have an empty map for props.tagOptions? I would guess 'no', right? Perhaps we need to validate that?

Because we iterate over the underlying values for creation and associating, if you do an empty one it will not fail and it won't create anything. I can add a test for this case as well, or since this it's mostly useless we could validate and fail.

It's better for this to fail since it wouldn't align with other constructs to instantiate one with a certain (empty) configuration that wouldn't yield anything in template. I've therefore added validation to make sure that the struct is not empty, and then that for each key the tag values are not empty, along with two tests for this. There is not a max length for these lists since I believe that these can be altered by the customer.

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

packages/@aws-cdk/aws-servicecatalog/test/tag-option.test.ts

Pull request has been modified.

skinny85

Looks great @arcrank. I basically have one last major question, and after that one is resolved, we can merge this one in.

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

skinny85 · 2022-01-21T23:44:04Z

packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts

- });
- };
+ for (const [tagOptionIdentifier, cfnTagOption] of Object.entries(tagOptions._tagOptionsMap)) {
+ const tagAssocationConstructId = `TagOptionAssociation${hashValues(resource.node.addr, tagOptionIdentifier)}`;


So, this is interesting - we are actually changing the algorithm used for generating the identifier of the CfnTagOptionAssociation resource. Previously, it was `TagOptionAssociation${hashValues(key, value, resource.node.addr)}` - now, it's `TagOptionAssociation${hashValues(resource.node.addr, tagOptionIdentifier)}`.

I wonder why this change? Wouldn't keeping the same algorithm reduce the number of changes in the template for customers upgrading to a new CDK version that will include these changes?

I have been able to maintain current behavior here. I didn't try to maintain this before because from my OOP life it felt wrong to add a more coupling between the two resources, and I more or less tracked the impact of changes that would happen as all or nothing - since we could expect to see the TagOption resources changing I thought it might just go hand in hand to expect associations to change as well.

If this new version is fine, my understanding is now better for it.

skinny85 · 2022-01-21T23:48:35Z

packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts

+ value: tagValue,
+ active: true,
+ });
+ tagOptionMap[tagOptionIdentifier] = tagOption;


Since tagOptionIdentifier is also the identifier of the CfnTagOption resource, is there actually any reason for this map? I think we can just have an array of CfnTagOption, and then retrieve its identifier with cfnTagOption.node.id (or possibly Node.of(cfnTagOption).id if it uses the new construct API).

I can just make this a list, the map part was a relic of an earlier point where we considered adding lookup access to an underlying tagOption(s), and a map felt more efficient than searching list.

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts

skinny85 · 2022-01-21T23:52:16Z

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts


 Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOption', 4); //Generates a resource for each unique key-value pair
- Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOptionAssociation', 6);


Can you instead keep key1 as having 2 values, and this way, we won't have to touch the assertions in this test?

good call out.

skinny85 · 2022-01-21T23:53:21Z

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

+ key1: ['value1'.repeat(1000), 'value2'],
+ key2: ['value1'],
+ },
+ });


I would add an empty line after this line.

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

skinny85

Looks beautiful @arcrank, awesome job!

mergify · 2022-01-24T23:23:18Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-01-25T02:43:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 5f1f4a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-01-25T02:43:16Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Fixes: [aws#17753](aws#17753) Previously TagOptions were defined via an interface and we only created the underlying resources upon an association. This broke CX if tagoptions were mangaged centrally. We move to make the TagOptions class a wrapper around aggregate individual TagOptions. BREAKING CHANGE: `TagOptions` now have `scope` and `props` argument in constructor, and data is now passed via a `allowedValueForTags` field in props ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot assigned skinny85 Jan 7, 2022

github-actions bot added the @aws-cdk/aws-servicecatalog Related to AWS Service Catalog label Jan 7, 2022

arcrank commented Jan 7, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts Outdated Show resolved Hide resolved

skinny85 previously requested changes Jan 12, 2022

View reviewed changes

skinny85 reviewed Jan 12, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/test/tag-option.test.ts Outdated Show resolved Hide resolved

addressing some comments

c3d930c

arcrank commented Jan 14, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/lib/product.ts Outdated Show resolved Hide resolved

arcrank commented Jan 14, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts Show resolved Hide resolved

arcrank commented Jan 14, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts Show resolved Hide resolved

arcrank commented Jan 14, 2022

View reviewed changes

packages/@aws-cdk/aws-servicecatalog/test/product.test.ts Show resolved Hide resolved

quick fix

023e440

arcrank commented Jan 14, 2022

View reviewed changes

Aidan Crank and others added 6 commits January 13, 2022 23:16

put modified tests back

d519888

Merge branch 'master' into tagOptionsConstruct

e6cc8cb

removing linting lines

fb125c1

Merging in changes from mainline

f67a303

fixing lint in package.json

d1cfcd4

make props struct

58e9391

arcrank requested a review from skinny85 January 18, 2022 15:42

skinny85 previously requested changes Jan 19, 2022

View reviewed changes

addressing comments

f33d001

Aidan Crank added 2 commits January 19, 2022 17:51

removing unintentional change

8c0f465

another unintentional typo

e434806

Aidan Crank and others added 4 commits January 20, 2022 09:44

one last doc cleanup

35e831f

add validation

03d733a

Merge branch 'master' into tagOptionsConstruct

e5bfdef

simplify iterating over unique tag values

e934d89

arcrank changed the title ~~feat(servicecatalog): Create standalone TagOptions Construct~~ feat(servicecatalog): Create TagOptions Construct Jan 21, 2022

arcrank requested a review from skinny85 January 21, 2022 18:19

skinny85 reviewed Jan 22, 2022

View reviewed changes

Aidan Crank added 2 commits January 24, 2022 10:39

maintain current behavior for association resource naming

ca9ec79

merging from master

84f9410

arcrank requested a review from skinny85 January 24, 2022 17:37

skinny85 approved these changes Jan 24, 2022

View reviewed changes

Merge branch 'master' into tagOptionsConstruct

5f1f4a6

mergify bot merged commit 903c4b6 into aws:master Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalog): Create TagOptions Construct #18314

feat(servicecatalog): Create TagOptions Construct #18314

arcrank commented Jan 7, 2022 •

edited

Loading

gitpod-io bot commented Jan 7, 2022

arcrank commented Jan 10, 2022

skinny85 left a comment

skinny85 commented Jan 12, 2022

arcrank Jan 14, 2022

skinny85 Jan 18, 2022

arcrank Jan 20, 2022

arcrank Jan 14, 2022 •

edited

Loading

arcrank Jan 14, 2022

skinny85 Jan 14, 2022

arcrank Jan 14, 2022

skinny85 left a comment

skinny85 Jan 18, 2022

arcrank Jan 19, 2022

skinny85 Jan 19, 2022

skinny85 Jan 18, 2022

arcrank Jan 19, 2022

arcrank Jan 20, 2022

skinny85 left a comment

skinny85 Jan 21, 2022

arcrank Jan 24, 2022

skinny85 Jan 21, 2022

arcrank Jan 24, 2022

skinny85 Jan 21, 2022

arcrank Jan 22, 2022

skinny85 Jan 21, 2022

skinny85 left a comment

mergify bot commented Jan 24, 2022

aws-cdk-automation commented Jan 25, 2022

mergify bot commented Jan 25, 2022

- * TagOption keys and associated list of possible values.
+ * The values that are allowed to be set for specific tags.
+ * The keys of the map are the names of the tags,
+ * and the values of the map are a list of allowed values of that particular tag.


		Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOption', 4); //Generates a resource for each unique key-value pair
		Template.fromStack(stack).resourceCountIs('AWS::ServiceCatalog::TagOptionAssociation', 6);

feat(servicecatalog): Create TagOptions Construct #18314

feat(servicecatalog): Create TagOptions Construct #18314

Conversation

arcrank commented Jan 7, 2022 • edited Loading

gitpod-io bot commented Jan 7, 2022

arcrank commented Jan 10, 2022

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcrank Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 24, 2022

aws-cdk-automation commented Jan 25, 2022

AWS CodeBuild CI Report

mergify bot commented Jan 25, 2022

arcrank commented Jan 7, 2022 •

edited

Loading

arcrank Jan 14, 2022 •

edited

Loading