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.

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: create a GuCertificateArnParameter construct #244

Merged
merged 2 commits into from
Feb 17, 2021
Merged

Conversation

akash1810
Copy link
Member

What does this change?

GuCertificateArnParameter has an allowed pattern of an ACM ARN string. Update GuHttpsApplicationListener to use it and validate the certificate prop too.

Does this change require changes to existing projects or CDK CLI?

No.

How to test

See updated tests and CI output.

How can we measure success?

Fewer errors as user input is restricted via the regex.

Have we considered potential risks?

n/a

GuCertificateArnParameter has an allowed pattern of an ACM ARN string.

Update GuHttpsApplicationListener to use it and validate the certificate prop too.
@akash1810 akash1810 requested a review from a team February 12, 2021 15:55
@akash1810
Copy link
Member Author

akash1810 commented Feb 12, 2021

I think it would be good to create the certificate ourselves. Imagine a construct/pattern with props that look like this:

interface Props {
  withLoadBalancer: true,
  withDomain: "foo.theguardian.com"
}

Then the construct/pattern will create an ACM resource. We probably need mappings to solve this as it needs to be stage aware, so leaving that for now.

},
},
],
});
});

test("passing in an invalid ACM ARN", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion, it might be nice to describe the expected behaviour in the test name?

Suggested change
test("passing in an invalid ACM ARN", () => {
test("throws an error if an invalid ACM ARN is passed in", () => {

@akash1810 akash1810 added the needs-new-release Identifying significant changes to the library label Feb 17, 2021
@akash1810 akash1810 merged commit 1058f4f into main Feb 17, 2021
@akash1810 akash1810 deleted the aa-acm-construct branch February 17, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-new-release Identifying significant changes to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants