add support for hostname annotation in ingress resource #545
Conversation
k8s-ci-robot
added
size/M
|
/retest |
source/ingress.go
Outdated
| @@ -232,6 +232,15 @@ func endpointsFromIngress(ing *v1beta1.Ingress) []*endpoint.Endpoint { | |||
| } | |||
| endpoints = append(endpoints, endpointsForHostname(rule.Host, targets, ttl)...) | |||
| } | |||
|
|
|||
| if ing.Annotations[hostnameAnnotationKey] != "" { | |||
| hostnames := strings.Split(ing.Annotations[hostnameAnnotationKey], ",") | |||
There was a problem hiding this comment.
the logic differs from https://github.com/kubernetes-incubator/external-dns/blob/68be609648b43dc067926979891bf5eb468b0dd3/source/service.go#L189, we should make it consistent (e.g. the code in service.go removes whitespace anywhere
Maybe extract into a new function and use it in both places?
k8s-ci-robot
added
size/L
|
Hi @hjacobs thank you for reviewing the code. I've made the suggested changes. seems like the build is failing due to timeout, I tried running on my laptop, it works just fine. can you please review again, and let me know if it looks ok now. Thanks |
|
@rajatjindal I just merged #548 which fixes the timeout problem with linters during the build --- if you rebase your PR, it should work 😄 |
|
@hjacobs rebase done, and that fixed the build too. thanks |
|
@linki @njuettner please have a quick look. LGTM. |
|
LGTM, thanks 👍 |
linki
added
the
lgtm
* Add index add command * Add validation around number of args * Prevent adding index if name is already used * Most code review changes Add success case to unit test and add initial integration test for index command * Update unit test Remove test table to make it easier to read and split into separate tests for failure and success cases * Code review changes * Check migration error * Code review changes * Code review changes Print empty index list and convert valid index name to use test tables * Update integration_test/index_test.go Co-Authored-By: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com> * Check number of lines in index integration test * Code review changes Use ListIndexes in add test and fix error message Co-authored-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
This PR fixes #402
We ran into similar issue where we had ingress rule hostname set to 'website.company.com' whereas the externaldns name we wanted was 'website.extdomain.company.com'
and website.company.com is a static cname to website.extdomain.company.com