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

aws-certificatemanager: DnsValidatedCertificate docs say that the default validation method is email #22739

Closed
plumdog opened this issue Nov 1, 2022 · 4 comments
Assignees
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2

Comments

@plumdog
Copy link
Contributor

plumdog commented Nov 1, 2022

Describe the issue

Documentation for the props forDnsValidatedCertificate say:

validation?

Type: CertificateValidation (optional, default: CertificateValidation.fromEmail())

How to validate this certificate.

which is obviously wrong, because its DNS validated.

I think this is just because interface DnsValidatedCertificateProps extends CertificateProps, and presumably this is a reasonable option in CertificateProps.

Solutions I can think of:

  • use some Omit<> to remove validation from DnsValidatedCertificateProps
  • set validation: never on DnsValidatedCertificateProps
  • factor out some BaseCertificateProps from CertificateProps that doesn't include validation and have DnsValidatedCertificateProps extend that instead.

Links

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_certificatemanager.DnsValidatedCertificate.html#validation

@plumdog plumdog added documentation This is a problem with documentation. needs-triage This issue or PR still needs to be triaged. labels Nov 1, 2022
@github-actions github-actions bot added the @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager label Nov 1, 2022
@peterwoodworth
Copy link
Contributor

The documentation is generated from our codebase - the line which this is coming from is here

* @default CertificateValidation.fromEmail()

As far as I can tell the validation prop isn't needed at all on DnsValidatedCertificate, it would be nice to not have props that do nothing. I think your third proposed solution would work well if it doesn't break anything 🙂

factor out some BaseCertificateProps from CertificateProps that doesn't include validation and have DnsValidatedCertificateProps extend that instead.

I am marking this issue as p2, which means that we are unable to work on this immediately.

We use +1s to help prioritize our work, and are happy to revaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

Check out our contributing guide if you're interested in contributing yourself - there's a low chance the team will be able to address this soon but we'd be happy to review a PR 🙂

@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 3, 2022
@plumdog
Copy link
Contributor Author

plumdog commented Nov 7, 2022

@peterwoodworth have had a go at this in #22809

@madeline-k
Copy link
Contributor

This has been addressed by #21982. Please open a new issue, if there is still documentation or features missing from the new construct: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_certificatemanager.Certificate.html

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants