feat(certificatemanager): allow tagging DnsValidatedCertificate #13990

timothy-farestad · 2021-04-06T04:41:04Z

Attempting to implement the fix suggested in #12382 to allow the DnsValidatedCertificate resource to be taggable. Currently, only the custom lambda that is created is tagged, but the certificate provisioned by the lambda is not tagged. This would allow the lambda to pass tags through to the certificate, too.

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

…tificates.

gitpod-io · 2021-04-06T04:43:30Z

njlynch

Looks great, thanks!

I left a few minor comments. Additionally, could you add two more tests:

A test for handler.test.js that shows success if tags are not provided.
A test for dns-validated-certificate.test.ts showing that tags are included as properties of the CustomResource if they are provided.

njlynch · 2021-04-13T09:13:33Z

...ws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js

+    const map = new Map(Object.entries(tags));
+    const result = Array.from(map).map(([Key, Value]) => ({ Key, Value }))


I believe the new Map here is redundant, unless there's a specific (edge) case I'm missing. The below should work:

Suggested change

const map = new Map(Object.entries(tags));

const result = Array.from(map).map(([Key, Value]) => ({ Key, Value }))

const result = Array.from(Object.entries(tags)).map(([Key, Value]) => ({ Key, Value }))

njlynch · 2021-04-13T09:17:58Z

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

@@ -34,7 +34,7 @@ See https://docs.aws.amazon.com/acm/latest/userguide/acm-limits.html for more in

 DNS validation is the preferred method to validate domain ownership, as it has a number of advantages over email validation.
 See also [Validate with DNS](https://docs.aws.amazon.com/acm/latest/userguide/gs-acm-validate-dns.html)
-in the AWS Certificate Manager User Guide.
+in the AWS Certificate Manager User Guide.  All stack tags will be applied to the DNS validated Certificate.


I think this is a little out of place here, and doesn't really need to be here. Our "all features must have README updates" rule is sometimes a bit aggressive.

Suggested change

in the AWS Certificate Manager User Guide. All stack tags will be applied to the DNS validated Certificate.

in the AWS Certificate Manager User Guide.

njlynch · 2021-04-13T09:19:43Z

packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts

+  /**
+ * Resource Tags.
+ * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-certificatemanager-certificate.html#cfn-certificatemanager-certificate-tags
+ */


Suggested change

/**

* Resource Tags.

* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-certificatemanager-certificate.html#cfn-certificatemanager-certificate-tags

*/

/**

* Resource Tags.

* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-certificatemanager-certificate.html#cfn-certificatemanager-certificate-tags

*/

njlynch · 2021-04-13T09:19:51Z

packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts

+
+

Suggested change

Pull request has been modified.

timothy-farestad · 2021-04-13T16:59:53Z

Really appreciate the review, @njlynch, I believe this latest commit addresses the tests as well as the syntax changes suggested.

packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts

njlynch

I made two other minor changes, and this is good to go!

Thanks for the contribution!

mergify · 2021-04-13T17:39:36Z

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

rix0rrr · 2021-04-15T07:39:44Z

@Mergifyio refresh

mergify · 2021-04-15T07:40:17Z

Command refresh: success

Pull request refreshed

mergify · 2021-04-15T09:49: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 · 2021-04-15T11:39:12Z

AWS CodeBuild CI Report

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

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

mergify · 2021-04-15T11:39:34Z

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

…13990) Closes aws#12382 Attempting to implement the fix suggested in aws#12382 to allow the DnsValidatedCertificate resource to be taggable. Currently, only the custom lambda that is created is tagged, but the certificate provisioned by the lambda is not tagged. This would allow the lambda to pass tags through to the certificate, too. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(certificatemanager): allow tags to be applied to dnsvalidatedcer…

84310a7

…tificates.

timothy-farestad mentioned this pull request Apr 6, 2021

(certificatemanager): DnsValidatedCertificate does not allow you to customize tags on the requested certificate #12382

Closed

2 tasks

Merge branch 'master' into master

6a7d283

timothy-farestad and others added 4 commits April 6, 2021 13:20

Merge branch 'master' into master

24a4c72

Update README

6f9540e

Merge branch 'master' of github.com:timothy-farestad/aws-cdk

6a89da0

Merge branch 'master' into master

322eb05

njlynch self-assigned this Apr 13, 2021

github-actions bot added the @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager label Apr 13, 2021

njlynch added effort/small Small work item – less than a day of effort p1 pr-linter/exempt-readme The PR linter will not require README changes and removed @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager labels Apr 13, 2021

njlynch changed the title ~~feat(certificatemanager): allow tags to be applied to dnsvalidatedcer…~~ feat(certificatemanager): allow tagging DnsValidatedCertificate Apr 13, 2021

github-actions bot added the @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager label Apr 13, 2021

njlynch previously requested changes Apr 13, 2021

View reviewed changes

added tests and cleanup of syntax as recommended by nlynch

334b856

njlynch reviewed Apr 13, 2021

View reviewed changes

packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts Outdated Show resolved Hide resolved

Two minor cleanups in the test

719ae49

njlynch approved these changes Apr 13, 2021

View reviewed changes

Merge branch 'master' into master

772955b

Merge branch 'master' into master

a977780

mergify bot merged commit 8360feb into aws:master Apr 15, 2021

henrist mentioned this pull request May 5, 2021

certificatemanager: DnsValidatedCertificate tags with cross-stack usage fails on upgrade to 1.100 #14519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(certificatemanager): allow tagging DnsValidatedCertificate #13990

feat(certificatemanager): allow tagging DnsValidatedCertificate #13990

timothy-farestad commented Apr 6, 2021 •

edited by njlynch

Loading

gitpod-io bot commented Apr 6, 2021 •

edited

Loading

njlynch left a comment

njlynch Apr 13, 2021

njlynch Apr 13, 2021

njlynch Apr 13, 2021

njlynch Apr 13, 2021

timothy-farestad commented Apr 13, 2021

njlynch left a comment

mergify bot commented Apr 13, 2021

rix0rrr commented Apr 15, 2021

mergify bot commented Apr 15, 2021

mergify bot commented Apr 15, 2021

aws-cdk-automation commented Apr 15, 2021

mergify bot commented Apr 15, 2021

		const map = new Map(Object.entries(tags));
		const result = Array.from(map).map(([Key, Value]) => ({ Key, Value }))

	const map = new Map(Object.entries(tags));
	const result = Array.from(map).map(([Key, Value]) => ({ Key, Value }))
	const result = Array.from(Object.entries(tags)).map(([Key, Value]) => ({ Key, Value }))

	in the AWS Certificate Manager User Guide. All stack tags will be applied to the DNS validated Certificate.
	in the AWS Certificate Manager User Guide.

feat(certificatemanager): allow tagging DnsValidatedCertificate #13990

feat(certificatemanager): allow tagging DnsValidatedCertificate #13990

Conversation

timothy-farestad commented Apr 6, 2021 • edited by njlynch Loading

gitpod-io bot commented Apr 6, 2021 • edited Loading

njlynch left a comment

Choose a reason for hiding this comment

njlynch Apr 13, 2021

Choose a reason for hiding this comment

njlynch Apr 13, 2021

Choose a reason for hiding this comment

njlynch Apr 13, 2021

Choose a reason for hiding this comment

njlynch Apr 13, 2021

Choose a reason for hiding this comment

timothy-farestad commented Apr 13, 2021

njlynch left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 13, 2021

rix0rrr commented Apr 15, 2021

mergify bot commented Apr 15, 2021

mergify bot commented Apr 15, 2021

aws-cdk-automation commented Apr 15, 2021

AWS CodeBuild CI Report

mergify bot commented Apr 15, 2021

timothy-farestad commented Apr 6, 2021 •

edited by njlynch

Loading

gitpod-io bot commented Apr 6, 2021 •

edited

Loading