-
Notifications
You must be signed in to change notification settings - Fork 27
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: add option to specify hosted zone ID #261
feat: add option to specify hosted zone ID #261
Conversation
For cases where the automatic discovery doesn't work (e.g. when we're creating a certificate for "foo.bar.com", and have a zone for "bar.com" that we can create the challenge records in). Signed-off-by: Andrew Dunham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, some comments below
provider.go
Outdated
// HostedZoneID is the ID of the hosted zone to use. If not set, it will | ||
// be discovered from the zone name. | ||
// | ||
// It must be in the format "/hostedzone/Z01111111111111111111". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very usable imo. Could you make this simply take the zone ID Z01111111111111111111
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aymanbagabas Changed to just take the zone ID and updated the comment; please take a look? I can squash the fixup commit if that looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I can squash the commit before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you @andrew-d
@andrew-d Since this also would affect caddy-dns/route53, could you send a PR to add the new option using Caddyfile? https://github.com/caddy-dns/route53/blob/master/route53.go#L62 |
For cases where the automatic discovery doesn't work (e.g. when we're creating a certificate for
foo.bar.com
, and have a zone forbar.com
that we can create the challenge records in).