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

feat(servicecatalog): Create TagOptions Construct #18314

Merged
merged 19 commits into from
Jan 25, 2022
Merged

feat(servicecatalog): Create TagOptions Construct #18314

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d0e30d3
feat(servicecatalog): Create standalone TagOptions Construct
Jan 7, 2022
c3d930c
addressing some comments
Jan 14, 2022
023e440
quick fix
Jan 14, 2022
d519888
put modified tests back
Jan 14, 2022
e6cc8cb
Merge branch 'master' into tagOptionsConstruct
arcrank Jan 14, 2022
fb125c1
removing linting lines
Jan 14, 2022
f67a303
Merging in changes from mainline
Jan 14, 2022
d1cfcd4
fixing lint in package.json
Jan 14, 2022
58e9391
make props struct
Jan 14, 2022
f33d001
addressing comments
Jan 19, 2022
8c0f465
removing unintentional change
Jan 19, 2022
e434806
another unintentional typo
Jan 19, 2022
35e831f
one last doc cleanup
Jan 20, 2022
03d733a
add validation
Jan 20, 2022
e5bfdef
Merge branch 'master' into tagOptionsConstruct
arcrank Jan 21, 2022
e934d89
simplify iterating over unique tag values
Jan 21, 2022
ca9ec79
maintain current behavior for association resource naming
Jan 24, 2022
84f9410
merging from master
Jan 24, 2022
5f1f4a6
Merge branch 'master' into tagOptionsConstruct
mergify[bot] Jan 25, 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
Prev Previous commit
Next Next commit
add validation
  • Loading branch information
Aidan Crank committed Jan 20, 2022
commit 03d733a3a6073e51d00111e17301db5a94f1aa82
13 changes: 9 additions & 4 deletions packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,20 @@ export class TagOptions extends cdk.Resource {

constructor(scope: Construct, id: string, props: TagOptionsProps) {
super(scope, id);
arcrank marked this conversation as resolved.
Show resolved Hide resolved

this._tagOptionsMap = this.createUnderlyingTagOptions(props.allowedValuesForTags);
}

private createUnderlyingTagOptions(tagOptions: { [tagKey: string]: string[] }): { [tagOptionIdentifier: string]: CfnTagOption } {
private createUnderlyingTagOptions(allowedValuesForTags: { [tagKey: string]: string[] }): { [tagOptionIdentifier: string]: CfnTagOption } {
if (Object.keys(allowedValuesForTags).length === 0) {
throw new Error(`No tag option keys or values were provided for resource ${this.node.path}`);
}
var tagOptionMap: { [tagOptionIdentifier: string]: CfnTagOption } = {};
for (const [tagKey, tagOptionsList] of Object.entries(tagOptions)) {
for (const [tagKey, tagValues] of Object.entries(allowedValuesForTags)) {
InputValidator.validateLength(this.node.addr, 'TagOption key', 1, 128, tagKey);
tagOptionsList.forEach((tagValue: string) => {
if (tagValues.length === 0) {
throw new Error(`No tag option values were provided for tag option key ${tagKey} for resource ${this.node.path}`);
}
tagValues.forEach((tagValue: string) => {
InputValidator.validateLength(this.node.addr, 'TagOption value', 1, 256, tagValue);
const tagOptionIdentifier = hashValues(tagKey, tagValue);
if (!this.node.tryFindChild(tagOptionIdentifier)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if doesn't make sense anymore, right? It can never be true anymore.

Pretty sure we should kill it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is carryover from how we originally did things so that if you for example had duplicate values it would be idempotent and ignore. I would prefer it if we could modify it so that creation is very strict, but that would change behavior again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this now to just get check for unique values, and we don't need to examine construct tree, which to some degree might have minor latency if construct tree is big.

Expand Down
19 changes: 19 additions & 0 deletions packages/@aws-cdk/aws-servicecatalog/test/tag-option.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,25 @@ describe('TagOptions', () => {
}).toThrowError(/Invalid TagOption value for resource/);
}),

test('fails to create tag options with no tag keys or values', () => {
expect(() => {
new servicecatalog.TagOptions(stack, 'TagOptions', {
allowedValuesForTags: {},
});
}).toThrowError(/No tag option keys or values were provided/);
}),

test('fails to create tag options for tag key with no values', () => {
expect(() => {
new servicecatalog.TagOptions(stack, 'TagOptions', {
allowedValuesForTags: {
key1: ['value1', 'value2'],
key2: [],
},
});
}).toThrowError(/No tag option values were provided for tag option key/);
}),

test('associate tag options', () => {
const portfolio = new servicecatalog.Portfolio(stack, 'MyPortfolio', {
displayName: 'testPortfolio',
Expand Down