Remove generator dependency of expose.go #106824

lauchokyip · 2021-12-05T04:53:04Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Remove the generators dependency from kubectl expose

Which issue(s) this PR fixes:

Fixes #93100

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Remove deprecated generator and container-port flags

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

lauchokyip · 2021-12-05T04:53:51Z

/cc @rikatz
/cc @soltysh

lauchokyip · 2021-12-05T04:55:50Z

/triage accepted
/priority important-longterm

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

lauchokyip · 2021-12-05T14:57:22Z

/retest

lauchokyip · 2021-12-05T17:43:26Z

@eddiezane Maybe you can help out for this one too?

deejross · 2021-12-20T17:09:59Z

/lgtm

k8s-ci-robot · 2021-12-20T17:10:21Z

@deejross: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

soltysh · 2021-12-21T09:38:45Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

-			}
+		protocolsMap, err := o.ProtocolsForObject(info.Object)
+		if err != nil {
+			return cmdutil.UsageErrorf(cmd, "couldn't find protocol via introspection: %v", err)


I'd prefer we switch all internal errors to just return error, so fmt.Errorf("couldn't find protocol via introspection: %v", err) in the rest of the file as well. This allows consumers of the command to wrap the error and respond accordingly. Whereas cmdutil.UsafeErrorf adds unnecessarily kubectl specific information, ie. See 'kubectl expose -h' for help and examples, etc, which are already nicely handled in the cobra's Run command.

soltysh · 2021-12-21T09:41:28Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

 			labels, err := meta.NewAccessor().Labels(info.Object)
 			if err != nil {
 				return err
 			}
-			params["labels"] = polymorphichelpers.MakeLabels(labels)
-		}
-		if err = generate.ValidateParams(names, params); err != nil {


service/v2 has two required values: default-name and selector see https://github.com/kubernetes/kubectl/blob/1bf219da9bdd8c03758195f5b43f1ddfbe31c3e7/pkg/generate/versioned/service.go#L55-L57 make sure to have appropriate Validate method to reflect that.

default-name cannot be moved to Validate because if user does not provide a name it will call the API server and get from the info object which mean user does not have to provide a name beforehand.
https://github.com/kubernetes/kubectl/blob/1bf219da9bdd8c03758195f5b43f1ddfbe31c3e7/pkg/cmd/expose/expose.go#L254

same as selector
https://github.com/kubernetes/kubectl/blob/1bf219da9bdd8c03758195f5b43f1ddfbe31c3e7/pkg/cmd/expose/expose.go#L263

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

soltysh · 2021-12-21T09:45:48Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

+	if len(o.Selector) == 0 {
+		return nil, fmt.Errorf("selector must be specified")
+	}
+	selector, err := parseLabels(o.Selector)


Probably worth moving that to Validate

See comment above

soltysh · 2021-12-21T09:45:57Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

+
+	var labels map[string]string
+	if len(o.Labels) > 0 {
+		labels, err = parseLabels(o.Labels)


Similarly, move to Validate.

See comment above

soltysh · 2021-12-21T09:46:22Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose_test.go

@@ -856,3 +858,878 @@ status:
 		})
 	}
 }
+
+func TestGenerateService(t *testing.T) {


Very good extensive tests 👍

lauchokyip · 2021-12-29T04:53:13Z

@soltysh , I don't think anything can be changed anymore. I have already thought of moving to Validate before submitting the PR but expose.go is different because almost all validation are done inside RunExpose

This PR preserves the intended behavior with generator without disrupting the flow of the code

dims · 2022-01-07T19:16:36Z

looks like @lauchokyip has responses for all the comments, back to approver!

/assign @soltysh

dims · 2022-01-07T19:16:47Z

/retest

soltysh

/lgtm
/approve

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

k8s-ci-robot · 2022-03-29T11:18:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lauchokyip, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2022-03-29T14:19:11Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-03-29T17:23:12Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

eddiezane · 2022-03-29T22:31:52Z

/retest

eddiezane · 2022-03-30T00:32:07Z

Flake is #108891

/retest

k8s-ci-robot · 2022-03-30T00:34:19Z

@lauchokyip: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-conformance-kind-ga-only-parallel	`d7ab8d4`	link	unknown	`/test pull-kubernetes-conformance-kind-ga-only-parallel`
pull-kubernetes-e2e-gce-ubuntu-containerd	`d7ab8d4`	link	unknown	`/test pull-kubernetes-e2e-gce-ubuntu-containerd`

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.

k8s-ci-robot requested review from brianpursley and dougsland December 5, 2021 04:53

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 5, 2021

k8s-ci-robot requested review from rikatz and soltysh December 5, 2021 04:53

lauchokyip force-pushed the removeExposeGenerator branch from 20bc407 to fadaf85 Compare December 5, 2021 13:46

lauchokyip commented Dec 5, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go Show resolved Hide resolved

lauchokyip commented Dec 5, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go Show resolved Hide resolved

soltysh requested changes Dec 21, 2021

View reviewed changes

Remove generator dependency of expose.go

d7ab8d4

lauchokyip force-pushed the removeExposeGenerator branch from fadaf85 to d7ab8d4 Compare December 29, 2021 04:48

lauchokyip requested a review from soltysh December 29, 2021 04:55

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 29, 2021

k8s-ci-robot assigned soltysh Jan 7, 2022

soltysh approved these changes Mar 29, 2022

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022

deejross mentioned this pull request Mar 29, 2022

REQUEST: New membership for deejross kubernetes/org#3344

Closed

9 tasks

k8s-ci-robot merged commit d429c98 into kubernetes:master Mar 30, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 30, 2022

github-actions bot mentioned this pull request Apr 5, 2022

Week Ending April 3, 2022 dev-obs/actus#414

Open

brianpursley mentioned this pull request Jan 9, 2023

fix: kubectl expose fails for apps with same-port, different-protocol #114909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove generator dependency of expose.go #106824

Remove generator dependency of expose.go #106824

lauchokyip commented Dec 5, 2021 •

edited by soltysh

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

deejross commented Dec 20, 2021

k8s-ci-robot commented Dec 20, 2021

soltysh Dec 21, 2021

lauchokyip Dec 29, 2021

soltysh Dec 21, 2021

lauchokyip Dec 29, 2021 •

edited

soltysh Dec 21, 2021

lauchokyip Dec 29, 2021

soltysh Dec 21, 2021

lauchokyip Dec 29, 2021

soltysh Dec 21, 2021

lauchokyip commented Dec 29, 2021 •

edited

dims commented Jan 7, 2022

dims commented Jan 7, 2022

soltysh left a comment

k8s-ci-robot commented Mar 29, 2022

k8s-triage-robot commented Mar 29, 2022

k8s-triage-robot commented Mar 29, 2022

eddiezane commented Mar 29, 2022

eddiezane commented Mar 30, 2022

k8s-ci-robot commented Mar 30, 2022

Remove generator dependency of expose.go #106824

Remove generator dependency of expose.go #106824

Conversation

lauchokyip commented Dec 5, 2021 • edited by soltysh

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

lauchokyip commented Dec 5, 2021

deejross commented Dec 20, 2021

k8s-ci-robot commented Dec 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip Dec 29, 2021 • edited

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

lauchokyip commented Dec 29, 2021 • edited

dims commented Jan 7, 2022

dims commented Jan 7, 2022

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 29, 2022

k8s-triage-robot commented Mar 29, 2022

k8s-triage-robot commented Mar 29, 2022

eddiezane commented Mar 29, 2022

eddiezane commented Mar 30, 2022

k8s-ci-robot commented Mar 30, 2022

lauchokyip commented Dec 5, 2021 •

edited by soltysh

lauchokyip Dec 29, 2021 •

edited

lauchokyip commented Dec 29, 2021 •

edited