fix(aws-certificatemanager): no unused props for DnsValidatedCertificate #22809
Conversation
|
|
github-actions
bot
added
beginning-contributor
| * | ||
| * @internal | ||
| */ | ||
| export interface BaseCertificateProps { |
There was a problem hiding this comment.
Q: should this be CertificateBaseProps? And does it belong in this file, or in certificate.ts?
There was a problem hiding this comment.
CertificateBaseProps. certificate-base.ts
| @@ -36,7 +36,6 @@ const hostedZone = PublicHostedZone.fromHostedZoneAttributes(stack, 'HostedZone' | |||
| const cert = new DnsValidatedCertificate(stack, 'Certificate', { | |||
| domainName, | |||
| hostedZone, | |||
| validation: CertificateValidation.fromDns(hostedZone), | |||
There was a problem hiding this comment.
This certainly feels like it should be redundant (as the fromDns is covered by virtue of being a DnsValidateCertificate, and the hostedZone is passed as its own prop). And couldn't see it getting used in dns-validated-certificate.ts anywhere.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
I don't think this warrants a change to an integration test. The fix is to remove some options that get ignored, so I don't think there's anything new to test, but rather just want for any existing tests to still pass. |
There was a problem hiding this comment.
This is a good change but unfortunately it is a breaking change. Anybody using DnsValidatedCertificate with validation is going to be broken by this.
#21982 will deprecate DnsValidatedCertificate, so we should ensure that this change gets added to the replacement construct when that is merged.
| * | ||
| * @internal | ||
| */ | ||
| export interface BaseCertificateProps { |
There was a problem hiding this comment.
CertificateBaseProps. certificate-base.ts
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
aws-cdk-automation
added
the
closed-for-staleness
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. |
Fixes #22739.
All Submissions:
Adding new Unconventional Dependencies:
n/a
New Features
n/a
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license