-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support for multiple hostnames in hostname annotation #256
Support for multiple hostnames in hostname annotation #256
Conversation
Thanks for the PR again 😄 (looks good to me so far, not sure if something is missing?) |
source/service.go
Outdated
endpoints = append(endpoints, endpoint.NewEndpoint(hostname, lb.Hostname, "")) | ||
hostnames := parseHostnameAnnotation(hostnameAnnotation) | ||
|
||
for _, hostname := range hostnames { |
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.
maybe inline?
for _, hostname := range parseHostnameAnnotation(hostnameAnnotation) {
LGTM But would like to note that if at some point we will be supporting custom TTL/Record Type per record configuration, we will have to change the specs format, as current |
Yes, totally makes sense. Is there a spec for this change? Maybe I'll implement it. |
@totallyunknown there is currently no clear defined specs, but some related issues: But it would be great if u create a proposal in #24 Let me have a final look at this and we can get this merged, IMO #24 should not be blocking this PR, at the end of the day once we graduate from alpha, we are free to use new annotation labels with the right format |
CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
- The flag `--domain-filter` can be repeated multiple times like `--domain-filter=example.com --domain-filter=company.org.`. | |||
- A trailing period is not required anymore for `--domain-filter` when AWS (or any other) provider is used. | |||
|
|||
- The `external-dns.alpha.kubernetes.io/hostname` annotation accepts now a comma separated list of hostnames and a trailing period is not required anymore. |
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.
nitpick: this should go on the top of the changelog. The idea is once release is prepared we will just have to add the date, and what follows is in the chronological order from most recent onwards
Can you please also update the docs? |
This doesn't seem to handle the compatibility case for route53 annotations. e.g. I have a service with
external-dns-0.4.1 with |
@kellycampbell You're right. We missed this case. I created a PR that should fix it: #301 Would you mind taking a look? |
…s#256) * service source: support for multiple hostnames per annotation * go fmt * Make parseHostnameAnnontations inline * Update CHANGELOG.md * Update Changelog
Setting multiple hostnames through this annotation has been possible since kubernetes-sigs#256, but this behavior has not been sufficiently documented before.
This is a PR for #197. Is there something missing? :)