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

add hostPorts to pod describer #57507

Merged
merged 1 commit into from Jan 16, 2018

Conversation

@dixudx
Member

dixudx commented Dec 21, 2017

What this PR does / why we need it:
Missing HostPorts when describing pods

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
/assign @mengqiy @shiywang
Release note:

None
if strings.Contains(hostPortString, ",") {
w.Write(LEVEL_2, "HostPorts:\t%s\n", hostPortString)
} else {
if len(hostPortString) == 1 {

This comment has been minimized.

@php-coder

php-coder Dec 21, 2017

Contributor

This condition looks weird. When the length of the string can be equal to 1?

This comment has been minimized.

@dixudx

dixudx Dec 22, 2017

Member

My mistake. Updated. PTAL. Thanks.

if len(hostPortString) == 0 {
w.Write(LEVEL_2, "HostPort:\t<none>\n")
} else {
w.Write(LEVEL_2, "HostPort:\t%s\n", hostPortString)

This comment has been minimized.

@php-coder

php-coder Dec 22, 2017

Contributor

I think that having more if conditions just for respecting plural form doesn't worth it. This code also could benefit from using stringOrNone():

w.Write(LEVEL_2, "HostPorts:\t%s\n", stringOrNone(hostPortString))

This comment has been minimized.

@dixudx

dixudx Dec 22, 2017

Member

@php-coder Excellent.

Updated. PTAL. Thanks.

@dixudx

This comment has been minimized.

Member

dixudx commented Dec 26, 2017

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 2, 2018

/assign juanvallejo

}
hostPortString := describeContainerHostPorts(container.Ports)
if strings.Contains(hostPortString, ",") {
w.Write(LEVEL_2, "HostPorts:\t%s\n", hostPortString)

This comment has been minimized.

@juanvallejo

juanvallejo Jan 8, 2018

Member

nit: s/HostPorts/Host Ports/

if strings.Contains(hostPortString, ",") {
w.Write(LEVEL_2, "HostPorts:\t%s\n", hostPortString)
} else {
w.Write(LEVEL_2, "HostPort:\t%s\n", stringOrNone(hostPortString))

This comment has been minimized.

@juanvallejo

juanvallejo Jan 8, 2018

Member

nit: s/HostPort/Host Port/

@juanvallejo

This comment has been minimized.

Member

juanvallejo commented Jan 8, 2018

@dixudx one nit, otherwise lgtm

@dixudx

This comment has been minimized.

Member

dixudx commented Jan 12, 2018

@juanvallejo @php-coder Updated. PTAL. Thanks.

@php-coder

This comment has been minimized.

Contributor

php-coder commented Jan 12, 2018

/lgtm

@dixudx

This comment has been minimized.

Member

dixudx commented Jan 12, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 16, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@soltysh

/lgtm

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 16, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 16, 2018

/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, dixudx, php-coder, soltysh

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 16, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 16, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 16, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit ff626a3 into kubernetes:master Jan 16, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation dixudx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@dixudx dixudx deleted the dixudx:describe_pod_hostport branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment