-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Clean up dynamic client pre-flight check #82652
Clean up dynamic client pre-flight check #82652
Conversation
Welcome @ricardomaraschini! |
Hi @ricardomaraschini. 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 Once the patch is verified, the new status will be reflected by the 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. |
/assign deads2k |
/cc |
@@ -691,6 +691,30 @@ func (r *Request) Stream() (io.ReadCloser, error) { | |||
} | |||
} | |||
|
|||
// requestPreflightCheck looks for common programmer errors on Request. | |||
// | |||
// We return an error if an attempt to POST is made using an "empty" namespace |
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.
We have cluster scoped resources like Nodes, ClusterRoles, and many others. This comment suggests that this change would prevent those from working.
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.
Well, maybe the comment is not clear enough. What I meant is that if r.namespaceSet == true
then len(r.namespace)
cant be zero(empty). This logic was already there, but in a different way[1].
[1] https://github.com/kubernetes/kubernetes/pull/82652/files#diff-a47b7155403b5094bbe75cf02d4da303L710
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.
@ricardomaraschini could you rewrite the comment to make that more obvious, please?
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.
Absolutely. It is done, ptal.
/hold I have reservations based on comment and where I see the code plumbed: https://github.com/kubernetes/kubernetes/pull/82652/files#r328611767 /ok-to-test I'm expecting failures. |
/test pull-kubernetes-integration |
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.
/priority important-longterm
/milestone v1.17
@@ -691,6 +691,30 @@ func (r *Request) Stream() (io.ReadCloser, error) { | |||
} | |||
} | |||
|
|||
// requestPreflightCheck looks for common programmer errors on Request. | |||
// | |||
// We return an error if an attempt to POST is made using an "empty" namespace |
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.
@ricardomaraschini could you rewrite the comment to make that more obvious, please?
/hold cancel The updated comment makes it more clear. I still haven't reviewed yet, but it no longer looks dangerous. |
@@ -702,19 +729,6 @@ func (r *Request) request(fn func(*http.Request, *http.Response)) error { | |||
metrics.RequestLatency.Observe(r.verb, r.finalURLTemplate(), time.Since(start)) | |||
}() | |||
|
|||
if r.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.
leave this in place.
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 have restored this to its original place on request()
instead of Do()
and DoRaw()
. Coming up on the next commit.
} | ||
} | ||
|
||
if err := r.requestPreflightCheck(); 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.
I like the previous location for this better. The only benefit I'm seeing here is gettnig ahead of the ratelimiter, but it opens us up to having to remember this construct if we ever have another Do or DoRaw. You can make the error typed if you like, but this duplicated construct doesn't look like a net improvement..
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.
Restored the default approach. Coming up with the next commit.
Migrated code that checks for common programmer errors to a separated function and added test coverage for it. Wrong comment stating that a typed error is returned was also removed.
/test pull-kubernetes-e2e-gce-100-performance |
/test pull-kubernetes-kubemark-e2e-gce-big |
this factorization is equivalent and cleaner, thanks. /lgtm |
/assign @alejandrox1 |
/assign @wojtek-t |
This looks good. Thank you @ricardomaraschini |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, ricardomaraschini 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 |
/test pull-kubernetes-e2e-gce |
@ricardomaraschini: The following test 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. |
…nstruction-error Clean up dynamic client pre-flight check
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This patch moves pre-request validations to a function and implements tests for it.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: