fix(core): tags not working for cognito user pools #4225

moofish32 · 2019-09-24T21:13:07Z

Cognito user pool tags do not use the property name Tags. In order to support properties without the name Tags the code needs to support arbitrary names for the tags field. I moved all references to Tags into the schema package such that codegen does not share any of the knowledge about special names for tag fields.

moved all knowledge about tag names into the schema package and
included UserPoolTags as taggable name
refactored codegen to use new schema package to identify tag
properties
TagManager now supports additional optional TagManagerOptions to control the naming of the tag property field and enable future extensions without breaking changes

Fixes #3882

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

* moved all knowledge about tag names into the schema package and included UserPoolTags as taggable name * refactored codegen to use new schema package to identify tag properties BREAKING CHANGE: * TagManager constructor now takes a property object instead of individual agruments: new TagManager(props: TagManagerProps) instead of new cdk.TagManager(cdk.TagType.STANDARD, resourceType, initialTags); Fixes aws#3882

mergify · 2019-09-24T21:13:44Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

moofish32 · 2019-09-24T21:17:52Z

packages/@aws-cdk/cfnspec/lib/schema/property.ts

+  UserPoolTags: "",
+};
+
+export type TagPropertyName = keyof typeof tagPropertyNames;


Not sure if there are reasons I can't do this, but it was the cleanest way to check a literal type. I suppose I could go enum?

Is JSII complaining about this?

export type TagPropertyName = 'Tags' | 'UserPoolTags';

If that's the case, I can see a slightly cleaner way that doesn't require instantiating an "unused" const:

interface TagProperty { Tags: never; UserPoolTags: never; } export type TagPropertyName = keyof TagProperty;

jsii didn't complain locally -- figured the full suite would tell me.

I meant complaining about my first snippet:

export type TagPropertyName = 'Tags' | 'UserPoolTags';

Sorry if I wasn't clear. And if you don't get errors building the cognito package, you should be good. 👍

packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts

aws-cdk-automation · 2019-09-24T21:39:03Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

moofish32 · 2019-09-24T21:52:38Z

is my fail just the breaking change? If so I can revert to avoid the break to one of the options identified above.

err  - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument tagType, takes @aws-cdk/core.TagManagerProps (formerly @aws-cdk/core.TagType): @aws-cdk/core.TagType is an enum different from @aws-cdk/core.TagManagerProps [incompatible-argument:@aws-cdk/core.TagManager.<initializer>]
err  - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument resourceTypeName, not accepted anymore. [removed-argument:@aws-cdk/core.TagManager.<initializer>]
err  - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument tagStructure, not accepted anymore. [removed-argument:@aws-cdk/core.TagManager.<initializer>]
@aws-cdk/custom-resources... OK.
@aws-cdk/cx-api... OK.
@aws-cdk/region-info... OK.
Some packages seem to have undergone breaking API changes. Please avoid.```

nmussy · 2019-09-24T21:57:38Z

Breaking changes are checked near the end of the build IIRC, so you should be good for the rest yes

…

On Tue, Sep 24, 2019, 11:52 PM Mike Cowgill ***@***.***> wrote: is my fail just the breaking change? If so I can revert to avoid the break to one of the options identified above. err - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument tagType, takes @aws-cdk/core.TagManagerProps (formerly @aws-cdk/core.TagType): @aws-cdk/core.TagType is an enum different from @aws-cdk/core.TagManagerProps ***@***.***/core.TagManager.<initializer>] err - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument resourceTypeName, not accepted anymore. ***@***.***/core.TagManager.<initializer>] err - INITIALIZER @aws-cdk/core.TagManager.<initializer>: argument tagStructure, not accepted anymore. ***@***.***/core.TagManager.<initializer>] @aws-cdk/custom-resources... OK. @aws-cdk/cx-api... OK. @aws-cdk/region-info... OK. Some packages seem to have undergone breaking API changes. Please avoid.``` — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4225?email_source=notifications&email_token=AATDXYHRJC3E6A3YOTPW2WTQLKD25A5CNFSM4I2E453KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7P5RLI#issuecomment-534763693>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATDXYBQGMYFMALT5LTB6ZTQLKD25ANCNFSM4I2E453A> .

eladb

In the PR description please include information on what was the issue and the root cause that motivated this change

packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts

packages/@aws-cdk/core/lib/tag-manager.ts

… checking

Pull request has been modified.

aws-cdk-automation · 2019-09-25T20:39:19Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-09-25T22:05:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

moofish32 · 2019-09-25T23:21:55Z

Where do I open the v2 issue or did I miss a document telling me about this?

aws-cdk-automation · 2019-10-08T20:51:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb · 2019-10-27T13:40:03Z

Hey @moofish32 what's up? How are you?

The v2.0 issue is here: https://github.com/aws/aws-cdk/issues/3398 (I pinned it now).
What's the next step here? Are you waiting for my review?

packages/@aws-cdk/core/test/test.tag-manager.ts

packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

moofish32 · 2019-10-28T04:37:10Z

@eladb - I refactored to make this backwards compatible. I think I'm ready for one more review I have a couple of clean up comments against my self. I'm still not sure how I should handle a breaking change in the constructor.

I went to an optional additional parameter:

constructor(tagType: TagType, resourceTypeName: string, tagStructure?: any, options?: TagManagerOptions)

Going forward it seems like it would make more sense to move the constructor to:

export interface TagManagerOptions {
  /**
   * The initial CloudFormation tags
   *
   * If the user set tags on the lower level Cfn* object this are passed here to
   * ensure they are set on the final contruct. Based on the TagType these are
   * validated and parsed.
   */
  readonly cfnTags?: any;
  /**
   * The name of the property in CloudFormation for these tags
   *
   * Normally this is Tags, but Cognito UserPool uses UserPoolTags
   * @default tags
   */
  readonly tagPropertyName?: string;
}


constructor(tagType: TagType, resourceTypeName: string, options?: TagManagerOptions)

Based on the V2 issue, how to deprecate a constructor.

packages/@aws-cdk/core/lib/tag-manager.ts

packages/@aws-cdk/core/test/test.tag-manager.ts

aws-cdk-automation · 2019-10-29T07:48:27Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-29T07:50:48Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-30T17:22:45Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-30T17:48:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-30T20:08:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

remove asCfnProperty and move logic to CfnResource

…s-cdk into f-add-userpool-tags

…ic in cfn-resource

Pull request has been modified.

aws-cdk-automation · 2020-01-02T00:37:23Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb · 2020-01-02T06:28:32Z

packages/@aws-cdk/core/lib/cfn-resource.ts

+      tagsProp[this.tags.tagPropertyName] = this.tags.renderTags();
+      return deepMerge(this._cfnProperties || {}, tagsProp);
+    }
+    return this._cfnProperties;


Suggested change

return this._cfnProperties;

return this._cfnProperties || {};

Pull request has been modified.

aws-cdk-automation · 2020-01-02T07:04:01Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

…s-cdk into f-add-userpool-tags

aws-cdk-automation · 2020-01-02T18:27:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

moofish32 · 2020-01-03T07:14:56Z

I believe I'm complete for all comments, let me know if I need to update branch or take any other action.

mergify · 2020-01-03T09:20:30Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-03T09:54:49Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-03T10:07:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-03T10:07:32Z

Thank you for contributing! Your pull request is now being automatically merged.

nmussy · 2024-04-16T15:11:42Z

3rd test body

moofish32 requested review from eladb and nija-at as code owners September 24, 2019 21:13

moofish32 mentioned this pull request Sep 24, 2019

Tags not applied to Cognito UserPools #3882

Closed

moofish32 commented Sep 24, 2019

View reviewed changes

packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts Outdated Show resolved Hide resolved

moofish32 changed the title ~~fix(core): Support tags for Cognito User Pools~~ fix(core): support tags for cognito user pools Sep 24, 2019

SomayaB assigned eladb Sep 24, 2019

eladb previously requested changes Sep 25, 2019

View reviewed changes

packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/lib/tag-manager.ts Show resolved Hide resolved

moving back to non-breaking change and simplifying the interface/type…

27204a6

… checking

fixing logic gap in tag type lookup

c8c306b

Merge branch 'master' into f-add-userpool-tags

7f92925

moofish32 commented Oct 28, 2019

View reviewed changes

packages/@aws-cdk/core/test/test.tag-manager.ts Outdated Show resolved Hide resolved

moofish32 commented Oct 28, 2019

View reviewed changes

packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts Outdated Show resolved Hide resolved

Elad Ben-Israel added 2 commits October 29, 2019 09:21

Update tag-manager.ts

c4c0531

Update tag-manager.ts

f018211

eladb previously requested changes Oct 29, 2019

View reviewed changes

packages/@aws-cdk/core/lib/tag-manager.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/test/test.tag-manager.ts Outdated Show resolved Hide resolved

Merge branch 'master' into f-add-userpool-tags

128b8f6

eladb previously requested changes Dec 31, 2019

View reviewed changes

moofish32 added 3 commits January 1, 2020 14:44

Merge branch 'master' into f-add-userpool-tags

e8a15d1

Merge branch 'f-add-userpool-tags' of https://github.com/moofish32/aw…

1b4c838

…s-cdk into f-add-userpool-tags

refactor(core): remove asCfnProperty from tag-manager and put the log…

b9e0200

…ic in cfn-resource

eladb changed the title ~~fix(core): support tags for cognito user pools~~ fix(core): tags not working for cognito user pools Jan 2, 2020

Update cfn-resource.ts

bcb963f

eladb previously approved these changes Jan 2, 2020

View reviewed changes

moofish32 added 3 commits January 2, 2020 09:49

Merge branch 'master' into f-add-userpool-tags

b91dc28

Merge branch 'f-add-userpool-tags' of https://github.com/moofish32/aw…

2a4e188

…s-cdk into f-add-userpool-tags

refactor(core): clean up cfnProperties to ensure undefined returns {}

3e761da

eladb approved these changes Jan 3, 2020

View reviewed changes

mergify bot added 2 commits January 3, 2020 09:21

Merge branch 'master' into f-add-userpool-tags

4756a45

Merge branch 'master' into f-add-userpool-tags

ffc3f51

mergify bot merged commit a67f0ef into aws:master Jan 3, 2020

moofish32 deleted the f-add-userpool-tags branch January 6, 2020 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): tags not working for cognito user pools #4225

fix(core): tags not working for cognito user pools #4225

moofish32 commented Sep 24, 2019 •

edited

Loading

mergify bot commented Sep 24, 2019

moofish32 Sep 24, 2019

nmussy Sep 24, 2019

moofish32 Sep 24, 2019

nmussy Sep 24, 2019

aws-cdk-automation commented Sep 24, 2019

moofish32 commented Sep 24, 2019

nmussy commented Sep 24, 2019 via email

eladb left a comment

aws-cdk-automation commented Sep 25, 2019

aws-cdk-automation commented Sep 25, 2019

moofish32 commented Sep 25, 2019

aws-cdk-automation commented Oct 8, 2019

eladb commented Oct 27, 2019

moofish32 commented Oct 28, 2019

aws-cdk-automation commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

aws-cdk-automation commented Dec 30, 2019

aws-cdk-automation commented Dec 30, 2019

aws-cdk-automation commented Dec 30, 2019

eladb left a comment

aws-cdk-automation commented Jan 2, 2020

eladb Jan 2, 2020

aws-cdk-automation commented Jan 2, 2020

aws-cdk-automation commented Jan 2, 2020

moofish32 commented Jan 3, 2020

mergify bot commented Jan 3, 2020

aws-cdk-automation commented Jan 3, 2020

aws-cdk-automation commented Jan 3, 2020

mergify bot commented Jan 3, 2020

nmussy commented Apr 16, 2024 via email

	return this._cfnProperties;
	return this._cfnProperties \|\| {};

fix(core): tags not working for cognito user pools #4225

fix(core): tags not working for cognito user pools #4225

Conversation

moofish32 commented Sep 24, 2019 • edited Loading

mergify bot commented Sep 24, 2019

moofish32 Sep 24, 2019

Choose a reason for hiding this comment

nmussy Sep 24, 2019

Choose a reason for hiding this comment

moofish32 Sep 24, 2019

Choose a reason for hiding this comment

nmussy Sep 24, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 24, 2019

AWS CodeBuild CI Report

moofish32 commented Sep 24, 2019

nmussy commented Sep 24, 2019 via email

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

moofish32 commented Sep 25, 2019

aws-cdk-automation commented Oct 8, 2019

AWS CodeBuild CI Report

eladb commented Oct 27, 2019

moofish32 commented Oct 28, 2019

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 30, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

eladb Jan 2, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

moofish32 commented Jan 3, 2020

mergify bot commented Jan 3, 2020

aws-cdk-automation commented Jan 3, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 3, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 3, 2020

nmussy commented Apr 16, 2024 via email

moofish32 commented Sep 24, 2019 •

edited

Loading