-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Remove generator dependency of expose.go #106824
Remove generator dependency of expose.go #106824
Conversation
/triage accepted |
20bc407
to
fadaf85
Compare
/retest |
@eddiezane Maybe you can help out for this one too? |
/lgtm |
@deejross: changing LGTM is restricted to collaborators In response to this:
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. |
} | ||
protocolsMap, err := o.ProtocolsForObject(info.Object) | ||
if err != nil { | ||
return cmdutil.UsageErrorf(cmd, "couldn't find protocol via introspection: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if len(o.Selector) == 0 { | ||
return nil, fmt.Errorf("selector must be specified") | ||
} | ||
selector, err := parseLabels(o.Selector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth moving that to Validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
|
||
var labels map[string]string | ||
if len(o.Labels) > 0 { | ||
labels, err = parseLabels(o.Labels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, move to Validate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
@@ -856,3 +858,878 @@ status: | |||
}) | |||
} | |||
} | |||
|
|||
func TestGenerateService(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good extensive tests 👍
fadaf85
to
d7ab8d4
Compare
@soltysh , I don't think anything can be changed anymore. I have already thought of moving to This PR preserves the intended behavior with generator without disrupting the flow of the code |
looks like @lauchokyip has responses for all the comments, back to approver! /assign @soltysh |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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:
Approvers can indicate their approval by writing |
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:
You can:
/retest |
1 similar comment
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:
You can:
/retest |
/retest |
@lauchokyip: The following tests failed, say
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. |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: