AWS: kubectl get service should print hostnames for LB services #26749
Conversation
k8s-github-robot
added
size/M
|
Rebased and squashed |
k8s-github-robot
removed
the
needs-rebase
| } | ||
| } | ||
| return strings.Join(result, ",") | ||
| r := strings.Join(result, ",") | ||
| if !wide && len(r) > 16 { |
There was a problem hiding this comment.
Nit: Wondering about the "magic constant" here... Is it defined somewhere else? Could we pass it in?
There was a problem hiding this comment.
It's defined in @thockin's original suggestion. :-) I could pass it in, but then I would have to define a constant somewhere else for the other callers to use and the function would go from one to three arguments.
|
This LGTM (though with one nit about the magic constant 16). It's annoying that it will never (?) be the full name, but I don't think there's anything we can realistically do about that without making the output super-wide. It does serve as a useful hint to people that there is a dns name, which will really help a lot of new users, as well as for experienced users to have a reminder that there is an ELB on certain services also. I'm going to put into the 1.3 milestone with a P2; it is an issue that has cropped up a lot so we might consider it a bug fix. |
justinsb
added
the
priority/backlog
|
I replaced the magic numeric constants with a symbolic one, but I'm not passing it in (yet). We could actually show two extra characters from the original string if we used U+2026 ( |
|
lgtm @ncdc what are the rules about the milestone? |
|
@smarterclayton @kubernetes/kubectl @kubernetes/rh-ux |
|
Milestone-wise, this fix looks concise enough to go in 1.3, especially if before we cut Beta on Friday. Who needs to review or apply the LGTM tag? |
|
I'll take anyone's LGTM. I'm not picky! |
j3ffml
added
lgtm
|
Please squash, and I'll add label. |
…ices Fixes kubernetes#21526 Also test wide outputs. We only guarantee the first IP to be fully printed if multiple ingresses are present. For AWS, which has no ingress IPs, but only hostnames, the ELB hostname will be truncated, unless -o=wide is specified.
|
Squashed. Sorry, I was at an offsite away from civilization the whole day. |
j3ffml
added
the
lgtm
|
GCE e2e build/test passed for commit 40f76a9. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 40f76a9. |
|
Automatic merge from submit-queue |
Fixes #21526
Also test wide outputs. We only guarantee the first IP to be fully printed
if multiple ingresses are present. For AWS, which has no ingress IPs, but
only hostnames, the ELB hostname will be truncated, unless -o=wide is
specified.