(#21648)add kubectl set port command #51947

zjj2wry · 2017-09-05T10:16:54Z

What this PR does / why we need it:
#21648
add kubectl set port command
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Add 'kubectl set port' command for setting port in resources embedding pod templates

k8s-ci-robot · 2017-09-05T10:17:02Z

Hi @zjj2wry. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-09-05T10:17:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zjj2wry
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

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

docs/OWNERS
pkg/kubectl/OWNERS

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

zjj2wry · 2017-09-05T11:10:32Z

cc @kubernetes/sig-cli-pr-reviews

k8s-ci-robot · 2017-09-05T11:10:40Z

@zjj2wry: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cli-pr-reviews.

In response to this:

cc @kubernetes/sig-cli-pr-reviews

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dixudx · 2017-09-05T15:11:19Z

pkg/kubectl/cmd/set/set_port.go

+	"fmt"
+	"io"
+	"strings"
+


remove this newline?

dixudx · 2017-09-05T15:11:44Z

pkg/kubectl/cmd/set/set_port.go

+
+	"strconv"
+
+	"github.com/spf13/cobra"


add a newline here?

dixudx · 2017-09-05T15:14:52Z

pkg/kubectl/cmd/set/set_port.go

+	cmd.Flags().BoolVar(&options.All, "all", false, "Select all resources in the namespace of the specified resource types")
+	cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)")
+	cmd.Flags().BoolVar(&options.Local, "local", false, "If true, set port will NOT contact api-server but run locally.")
+	cmd.Flags().Int("port", 80, i18n.T("Name or number for the port on the container that the service should direct traffic to. Optional."))


port should be marked as required? I think we'd better not set default value, especially 80. WDYT?

yes, i will updated

dixudx · 2017-09-05T15:15:30Z

pkg/kubectl/cmd/set/set_port.go

+	}
+
+	cmd := &cobra.Command{
+		Use:     "port (-f FILENAME | TYPE NAME)",


appending --port ?

dixudx · 2017-09-05T15:16:06Z

pkg/kubectl/cmd/set/set_port.go

+
+	cmd := &cobra.Command{
+		Use:     "port (-f FILENAME | TYPE NAME)",
+		Short:   i18n.T("Update port of a pod template"),


What about Update existing container port(s) of resources ?

dixudx · 2017-09-05T15:27:46Z

pkg/kubectl/cmd/set/set_port.go

+			if len(containers) == 0 {
+				fmt.Println(info.Mapping.Resource, info.Name, o.containerSelector)
+				_, err := fmt.Fprintf(o.Err, "warning: %s/%s does not have any containers matching %q\n", info.Mapping.Resource, info.Name, o.containerSelector)
+				if err != nil {


what about

if _, err := xxx; err != nil { return err }

dixudx · 2017-09-05T15:30:42Z

pkg/kubectl/cmd/set/set_port.go

+				return err
+			}
+			for _, c := range containers {
+				if !o.overwrite {


L199-L205 seems have nothing to do with c. Can we move it to outer?

yes, thank you point it

dixudx · 2017-09-05T15:32:01Z

pkg/kubectl/cmd/set/set_port.go

+			if _, err := fmt.Fprintln(o.Out, "no resources changed"); err != nil {
+				return nil, err
+			}
+			return nil, err


return nil, err ?

yes, this should be return nil,nil

dixudx · 2017-09-05T15:32:59Z

pkg/kubectl/cmd/set/set_port.go

+	for _, patch := range patches {
+		info := patch.Info
+		if patch.Err != nil {
+			allErrs = append(allErrs, fmt.Errorf("error: %s/%s %v\n", info.Mapping.Resource, info.Name, patch.Err))


not sure for adding \n here.

yes, this should

dixudx · 2017-09-05T15:33:45Z

pkg/kubectl/cmd/set/set_port.go

+		}
+
+		// no changes
+		if string(patch.Patch) == "{}" || len(patch.Patch) == 0 {


should have a better way of string(patch.Patch) == "{}".

this follow other set command

zjj2wry · 2017-09-05T16:31:55Z

@dixudx thanks, i will fix this tomorrow

zjj2wry

@dixudx thank your careful review work. i will fix as soon as possible

zjj2wry · 2017-09-06T15:39:55Z

pkg/kubectl/cmd/set/set_port.go

+	o.Output = cmdutil.GetFlagString(cmd, "output")
+	o.port = cmdutil.GetFlagInt(cmd, "port")
+	o.protocol = strings.ToUpper(cmdutil.GetFlagString(cmd, "protocol"))
+	o.containerSelector = cmdutil.GetFlagString(cmd, "containers")


i parse this in L184

zjj2wry · 2017-09-06T15:40:45Z

pkg/kubectl/cmd/set/set_port.go

+	o.Local = cmdutil.GetFlagBool(cmd, "local")
+	o.DryRun = cmdutil.GetDryRunFlag(cmd)
+	o.Output = cmdutil.GetFlagString(cmd, "output")
+	o.port = cmdutil.GetFlagInt(cmd, "port")


not sure, but i'd more like keep a style.

zjj2wry · 2017-09-06T15:41:15Z

pkg/kubectl/cmd/set/set_port.go

+	cmd.Flags().BoolVar(&options.All, "all", false, "Select all resources in the namespace of the specified resource types")
+	cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)")
+	cmd.Flags().BoolVar(&options.Local, "local", false, "If true, set port will NOT contact api-server but run locally.")
+	cmd.Flags().Int("port", 80, i18n.T("Name or number for the port on the container that the service should direct traffic to. Optional."))


yes, i will updated

zjj2wry · 2017-09-06T15:42:26Z

pkg/kubectl/cmd/set/set_port.go

+	if o.protocol != "TCP" && o.protocol != "UDP" {
+		errors = append(errors, fmt.Errorf("protocol must tcp or udp"))
+	}
+	if o.port < 0 {


agree, i will follow server-side

zjj2wry · 2017-09-06T15:42:44Z

pkg/kubectl/cmd/set/set_port.go

+			if len(containers) == 0 {
+				fmt.Println(info.Mapping.Resource, info.Name, o.containerSelector)
+				_, err := fmt.Fprintf(o.Err, "warning: %s/%s does not have any containers matching %q\n", info.Mapping.Resource, info.Name, o.containerSelector)
+				if err != nil {


zjj2wry · 2017-09-06T15:43:03Z

pkg/kubectl/cmd/set/set_port.go

+				return err
+			}
+			for _, c := range containers {
+				if !o.overwrite {


yes, thank you point it

zjj2wry · 2017-09-06T15:43:27Z

pkg/kubectl/cmd/set/set_port.go

+			if _, err := fmt.Fprintln(o.Out, "no resources changed"); err != nil {
+				return nil, err
+			}
+			return nil, err


yes, this should be return nil,nil

dixudx · 2017-09-07T13:38:51Z

pkg/kubectl/cmd/set/set_port.go

+	return nil
+}
+
+func (o *PortOptions) Validate() error {


port (-f FILENAME | TYPE NAME) [--port=PORT]

I think we'd better add validation on the type here.

this validation in f.UpdatePodSpecForObject(info.Object, func(spec *api.PodSpec) error

dixudx · 2017-09-07T13:51:05Z

pkg/kubectl/cmd/set/set_port.go

+}
+
+var (
+	portResources = `


Should we add like this? This does not seems like k8s style.

Please refer below,

kubernetes/pkg/kubectl/cmd/cmd.go

Lines 202 to 242 in ea01771

validResources = `Valid resource types include:

* all

* certificatesigningrequests (aka 'csr')

* clusterrolebindings

* clusterroles

* clusters (valid only for federation apiservers)

* componentstatuses (aka 'cs')

* configmaps (aka 'cm')

* controllerrevisions

* cronjobs

* customresourcedefinition (aka 'crd')

* daemonsets (aka 'ds')

* deployments (aka 'deploy')

* endpoints (aka 'ep')

* events (aka 'ev')

* horizontalpodautoscalers (aka 'hpa')

* ingresses (aka 'ing')

* jobs

* limitranges (aka 'limits')

* namespaces (aka 'ns')

* networkpolicies (aka 'netpol')

* nodes (aka 'no')

* persistentvolumeclaims (aka 'pvc')

* persistentvolumes (aka 'pv')

* poddisruptionbudgets (aka 'pdb')

* podpreset

* pods (aka 'po')

* podsecuritypolicies (aka 'psp')

* podtemplates

* replicasets (aka 'rs')

* replicationcontrollers (aka 'rc')

* resourcequotas (aka 'quota')

* rolebindings

* roles

* secrets

* serviceaccounts (aka 'sa')

* services (aka 'svc')

* statefulsets

* storageclasses

`

But we're trying to replace such hard-coded way, refer to #42873 and #51324.

@liggitt PTAL. For updating such container ports, can we use the old hard-coded strings.

this mean support update env's resources

zjj2wry · 2017-09-24T14:49:50Z

/kind feature

zjj2wry · 2017-09-25T11:38:01Z

/unassign @dshulyak
/assign @liggitt

liggitt · 2017-09-25T13:41:25Z

/unassign
/assign @fabianofranz @pwittrock

fabianofranz · 2017-10-06T16:41:17Z

/ok-to-test

k8s-ci-robot · 2017-10-06T17:20:42Z

@zjj2wry: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	`f0f1bd0`	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-gpu	`f0f1bd0`	link	`/test pull-kubernetes-e2e-gce-gpu`
pull-kubernetes-bazel-build	`f0f1bd0`	link	`/test pull-kubernetes-bazel-build`
pull-kubernetes-kubemark-e2e-gce	`f0f1bd0`	link	`/test pull-kubernetes-kubemark-e2e-gce`
pull-kubernetes-e2e-kops-aws	`f0f1bd0`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-bazel-test	`f0f1bd0`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-unit	`f0f1bd0`	link	`/test pull-kubernetes-unit`
pull-kubernetes-e2e-gce-etcd3	`f0f1bd0`	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-verify	`f0f1bd0`	link	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pwittrock · 2017-10-11T22:52:36Z

pkg/kubectl/cmd/set/set_port.go

+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/kubernetes/pkg/api"


We are not introducing new deps on k8s.io/kubernetes. How can this PR be done without this? Can we use the versioned objects.

we want to do this anyway, see the TODO in https://github.com/kubernetes/kubernetes/pull/53158/files#diff-466503ed5c81843c5ba705bc4c6fd4dc

// TODO: switch UpdatePodSpecForObject to work on v1.PodSpec, use info.VersionedObject, and avoid conversion completely

Why a TODO? Can we do this now before this PR goes in? We are actively trying to remove these dependencies from kubectl.

agree, was saying we could use this as an opportunity to resolve the TODO

oh perfect :)

there is a issue created by wangshiyang. will help fix it priority then fix this PR

open PR #54554 fix this

liggitt · 2017-10-12T04:03:05Z

pkg/kubectl/cmd/set/set_port.go

+			return nil, err
+		}
+		if transformed && err == nil {
+			return runtime.Encode(o.Encoder, info.Object)


rework this with the change from https://github.com/kubernetes/kubernetes/pull/53158/files#diff-466503ed5c81843c5ba705bc4c6fd4dc

as is, this would fail intermittently on DaemonSet and ReplicaSet objects

@liggitt nice, thank you very much.

liggitt · 2017-10-12T04:06:17Z

needs tests in test-cmd-util.sh on the various resources

zjj2wry · 2017-10-12T12:58:43Z

will updated it quickly

pwittrock · 2017-10-16T17:15:10Z

/assign @pwittrock

pwittrock · 2017-10-19T17:17:49Z

/unassign @pwittrock

k8s-github-robot · 2017-11-25T08:15:25Z

This PR hasn't been active in 30 days. It will be closed in 59 days (Jan 23, 2018).

cc @fabianofranz @seans3 @soltysh @zjj2wry

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

fabianofranz · 2017-12-21T17:06:50Z

/unassign

zjj2wry · 2018-02-22T05:45:35Z

/close

zhengjiajin added 2 commits September 5, 2017 18:15

(kubernetes#21648)add kubectl set port command

06f26c7

auto generated for kubectl set port command

c42f901

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 5, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 5, 2017

zjj2wry changed the title ~~Set port~~ (#21648)add kubectl set port command Sep 5, 2017

k8s-github-robot assigned dshulyak and soltysh Sep 5, 2017

k8s-github-robot added kind/old-docs release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 5, 2017

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Sep 5, 2017

dixudx reviewed Sep 5, 2017

View reviewed changes

zjj2wry commented Sep 6, 2017

View reviewed changes

update kubectl set port command default port value

f0f1bd0

dixudx reviewed Sep 7, 2017

View reviewed changes

zjj2wry mentioned this pull request Sep 11, 2017

Proposal: First-generation of kubectl set * commands #21648

Closed

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 24, 2017

k8s-ci-robot assigned liggitt and unassigned dshulyak Sep 25, 2017

k8s-ci-robot assigned fabianofranz and pwittrock and unassigned liggitt Sep 25, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 6, 2017

pwittrock assigned seans3 and unassigned pwittrock Oct 11, 2017

pwittrock reviewed Oct 11, 2017

View reviewed changes

liggitt reviewed Oct 12, 2017

View reviewed changes

k8s-ci-robot assigned pwittrock Oct 16, 2017

k8s-ci-robot unassigned pwittrock Oct 19, 2017

k8s-ci-robot unassigned fabianofranz Dec 21, 2017

k8s-ci-robot closed this Feb 22, 2018

	validResources = `Valid resource types include:

	* all
	* certificatesigningrequests (aka 'csr')
	* clusterrolebindings
	* clusterroles
	* clusters (valid only for federation apiservers)
	* componentstatuses (aka 'cs')
	* configmaps (aka 'cm')
	* controllerrevisions
	* cronjobs
	* customresourcedefinition (aka 'crd')
	* daemonsets (aka 'ds')
	* deployments (aka 'deploy')
	* endpoints (aka 'ep')
	* events (aka 'ev')
	* horizontalpodautoscalers (aka 'hpa')
	* ingresses (aka 'ing')
	* jobs
	* limitranges (aka 'limits')
	* namespaces (aka 'ns')
	* networkpolicies (aka 'netpol')
	* nodes (aka 'no')
	* persistentvolumeclaims (aka 'pvc')
	* persistentvolumes (aka 'pv')
	* poddisruptionbudgets (aka 'pdb')
	* podpreset
	* pods (aka 'po')
	* podsecuritypolicies (aka 'psp')
	* podtemplates
	* replicasets (aka 'rs')
	* replicationcontrollers (aka 'rc')
	* resourcequotas (aka 'quota')
	* rolebindings
	* roles
	* secrets
	* serviceaccounts (aka 'sa')
	* services (aka 'svc')
	* statefulsets
	* storageclasses
	`

(#21648)add kubectl set port command #51947

(#21648)add kubectl set port command #51947

Conversation

zjj2wry commented Sep 5, 2017 • edited Loading

k8s-ci-robot commented Sep 5, 2017

k8s-github-robot commented Sep 5, 2017

zjj2wry commented Sep 5, 2017

k8s-ci-robot commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry commented Sep 5, 2017

zjj2wry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry commented Sep 24, 2017

zjj2wry commented Sep 25, 2017

liggitt commented Sep 25, 2017

fabianofranz commented Oct 6, 2017

k8s-ci-robot commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 12, 2017

zjj2wry commented Oct 12, 2017

pwittrock commented Oct 16, 2017

pwittrock commented Oct 19, 2017

k8s-github-robot commented Nov 25, 2017

fabianofranz commented Dec 21, 2017

zjj2wry commented Feb 22, 2018

zjj2wry commented Sep 5, 2017 •

edited

Loading

zjj2wry Oct 20, 2017 •

edited

Loading