feat(servicecatalog): allow associating TagOptions with a Portfolio by wanjacki · Pull Request #15612 · aws/aws-cdk

wanjacki · 2021-07-16T19:28:36Z

Allows users to add TagOptions to their portfolio.

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

gitpod-io · 2021-07-16T19:28:38Z

skinny85

Looks good, but the API needs some polish before we can merge it in.

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

skinny85 · 2021-07-16T21:15:52Z

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

 portfolio.addProduct(product);
 ```

+### TagOptions


Suggested change

### TagOptions

### Tag options

skinny85 · 2021-07-16T21:16:12Z

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

+TagOptions allow administrators to easily manage tags on provisioned products by creating a selection of tags for end users to choose from.
+For example, an end user can choose an `ec2` for the instance type size.
+TagOptions are created by specifying a key with a selection of values.
+At the moment, TagOptions can only be disabled on the console.


Suggested change

At the moment, TagOptions can only be disabled on the console.

At the moment, TagOptions can only be disabled in the console.

skinny85 · 2021-07-16T21:19:09Z

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

+   * A TagOption is a key-value pair managed in AWS Service Catalog.
+   * It is not an AWS tag, but serves as a template for creating an AWS tag based on the TagOption.
+   */
+  addTagOptions(tagOptions: TagOptions): void;


Let's also allow setting the TagOptions directly when creating the Portfolio. So, add an optional readonly tagOptions?: TagOptions property to PortfolioProps.

Since you're using "associate" in the description already, how about we call this method associateTagOptions()? I'm worried we're overloading the term "add" in this class to mean very many different things.

skinny85 · 2021-07-16T21:21:36Z

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

    }
  }

+  public static associateTagOptions(portfolio: IPortfolio, resourceId: string, tagOptions: TagOptions) {


I don't think resourceId as a separate parameter makes too much sense. I think you should remove it, and use portfolio.portfolioId in the expression below in CfnTagOptionAssociation.

Specify the return type of this function explicitly please.

skinny85 · 2021-07-16T21:22:22Z

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

@@ -0,0 +1,35 @@
+import * as cdk from '@aws-cdk/core';
+import { CfnTagOption } from '.';


This is a weird notation - I wouldn't use it.

skinny85 · 2021-07-16T21:24:00Z

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

@@ -0,0 +1,35 @@
+import * as cdk from '@aws-cdk/core';


This file should be called tag-options.ts.

skinny85 · 2021-07-16T21:26:28Z

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

+  /**
+  * List of CfnTagOption
+  */
+  public readonly cfnTagOptions: CfnTagOption[];


We don't use properties from the L1 layer in publicly-available classes like this.

Rename the property to tagOptions, and make it return a {[key: string]: string[]} (make sure it's a defensive copy though, so none can mutate them in that way). Then, move all of the logic of creating CfnTagOptions into the associateTagOptions() method of AssociationManager.

The reason why the logic is here is because tagOptions are global. Moving it to associateTagOptions would require us to create the L1 when an association is made and do an additional check each time to make sure no duplicate tagOptions are being created. This brings up the issue of what happens when we create one tagOption and apply it to two portfolio, it would create two tagOptions (unless we maintain a map of tagOptions as using tryFindChild won't work due to the different hash/portfolio).
What do you recommend Adam?

You can still make them global by retrieving the Stack a given Portfolio belongs to with the Stack.of(portfolio) method.

skinny85 · 2021-07-16T21:27:23Z

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

+  */
+  public readonly cfnTagOptions: CfnTagOption[];
+
+  constructor(stack : cdk.Stack, tagOptionsMap: {[key: string]: string[]}) {


This is not really an API we would consider adding to the CDK - it's very unnatural.

Get rid of the stack parameter from here.

skinny85 · 2021-07-16T21:29:09Z

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

+  }),
+
+  test('fails to add tag options with invalid value length', () => {
+


No need for this empty line.

Pull request has been modified.

skinny85

Looks great @wanjacki! A few last polishes before we merge this in.

skinny85 · 2021-07-19T19:13:12Z

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

+  key1: ['value1', 'value2'],
+  key2: ['value1'],


Can we give a more realistic example here? Perhaps the one using the EC2 instance sizes mentioned above?

skinny85 · 2021-07-19T19:15:13Z

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

+        const stack = cdk.Stack.of(portfolio);
+        const tagOptionKey = hashValues(key, value, stack.node.addr);
+        const tagOptionConstructId = `TagOption${tagOptionKey}`;
+        var cfnTagOption = stack.node.tryFindChild(tagOptionConstructId) as CfnTagOption;


var -> let

skinny85 · 2021-07-19T19:15:53Z

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

+  /**
+  * List of CfnTagOption
+  */
+  public readonly tagOptionsMap: {[key: string]: string[]};


Suggested change

public readonly tagOptionsMap: {[key: string]: string[]};

public readonly tagOptionsMap: { [key: string]: string[] };

skinny85 · 2021-07-19T19:16:39Z

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

+      InputValidator.validateLength(portfolio.node.addr, 'TagOption key', 1, 128, key);
+      tagOptions.tagOptionsMap[key].forEach((value: string) => {
+        InputValidator.validateLength(portfolio.node.addr, 'TagOption value', 1, 256, value);
+        const stack = cdk.Stack.of(portfolio);


Let's rename this variable to portfolioStack.

Also, let's move this out of the loop (it should be done once per associateTagOptions() call, not on each loop iteration).

skinny85 · 2021-07-19T19:19:39Z

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

+  public readonly tagOptionsMap: {[key: string]: string[]};
+
+  constructor(tagOptionsMap: {[key: string]: string[]}) {
+    this.tagOptionsMap = tagOptionsMap;


Like I wrote before, I think this should save a defensive copy of the passed map:

Suggested change

this.tagOptionsMap = tagOptionsMap;

this.tagOptionsMap = { ...tagOptionsMap };

So that mutating the tagOptionsMap after creating TagOptions does not cause any changes in the instance.

skinny85 · 2021-07-19T19:22:38Z

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

  }

+  public static associateTagOptions(portfolio: IPortfolio, tagOptions: TagOptions): void {
+    Object.keys(tagOptions.tagOptionsMap).forEach(key => {


You can use Object.entries with for...of instead, like this, which will save you accessing the value separately with tagOptions.tagOptionsMap[key] below.

Pull request has been modified.

aws-cdk-automation · 2021-07-19T22:16:09Z

AWS CodeBuild CI Report

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

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

skinny85

Looks great @wanjacki!

mergify · 2021-07-19T22:19:22Z

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

Allows users to add TagOptions to their portfolio. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

wanjacki and others added 2 commits July 16, 2021 11:19

feat(servicecatalog): Add TagOptions for portfolio

1699cff

Merge branch 'aws:master' into tagOptions

34e0735

wanjacki changed the title ~~Tag options~~ feat(servicecatalog): Add TagOptions for portfolio Jul 16, 2021

skinny85 previously requested changes Jul 16, 2021

View reviewed changes

wanjacki added 2 commits July 16, 2021 17:05

feat(servicecatalog): Addressed comments

8ab4430

Merge branch 'tagOptions' of github.com:wanjacki/aws-cdk into tagOptions

a2fc225

feat(servicecatalog): minor fix of portfolio test

a05e896

wanjacki requested a review from skinny85 July 17, 2021 00:16

Merge branch 'master' into tagOptions

f15a462

skinny85 previously requested changes Jul 19, 2021

View reviewed changes

wanjacki added 2 commits July 19, 2021 14:37

Addressed most recent comments

ac84386

Merge branch 'tagOptions' of github.com:wanjacki/aws-cdk into tagOptions

c59f455

wanjacki requested a review from skinny85 July 19, 2021 21:39

skinny85 approved these changes Jul 19, 2021

View reviewed changes

mergify bot merged commit e7760ee into aws:master Jul 19, 2021

skinny85 changed the title ~~feat(servicecatalog): Add TagOptions for portfolio~~ feat(servicecatalog): allow associating TagOptions with a Portfolio Jul 19, 2021

skinny85 mentioned this pull request Jul 20, 2021

feat(servicecatalog): Add stack event notification constraint #15610

Merged

	At the moment, TagOptions can only be disabled on the console.
	At the moment, TagOptions can only be disabled in the console.

		@@ -0,0 +1,35 @@
		import * as cdk from '@aws-cdk/core';
		import { CfnTagOption } from '.';

		}),

		test('fails to add tag options with invalid value length', () => {

	public readonly tagOptionsMap: {[key: string]: string[]};
	public readonly tagOptionsMap: { [key: string]: string[] };

	this.tagOptionsMap = tagOptionsMap;
	this.tagOptionsMap = { ...tagOptionsMap };

Conversation

wanjacki commented Jul 16, 2021

Allows users to add TagOptions to their portfolio.

Uh oh!

gitpod-io bot commented Jul 16, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jul 19, 2021

AWS CodeBuild CI Report

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants