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
Minor cleanups in watch handler toward unification between http and websockets #122115
Minor cleanups in watch handler toward unification between http and websockets #122115
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Nov 29 16:05:22 UTC 2023. |
b608b51
to
1fd1570
Compare
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.
Thanks for cleaning this up. Happy to approver after the two (minor) comments I've added are cleared.
@@ -265,7 +264,7 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
} | |||
} | |||
|
|||
// HandleWS implements a websocket handler. | |||
// HandleWs serves a series of encoded events over a websocket connection. |
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.
// HandleWs serves a series of encoded events over a websocket connection. | |
// HandleWS serves a series of encoded events over a websocket connection. |
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.
done
return | ||
} | ||
|
||
func (s *WatchServer) HandleHTTP(w http.ResponseWriter, req *http.Request) { |
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.
Is this rename worth it? It's an exported symbol in a staging repo..
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 changed slightly what it's doing (e.g. it's not handling websocket now), so changing name helps imho [I doubt someone vendoring this repo is really using that).
But if you feel strongly about leaving the name as it was previously - I can revert it.
/approve |
LGTM label has been added. Git tree hash: bf799fc1f44d2878402c4f7cc8b234c4d552e8ac
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, wojtek-t 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 |
1fd1570
to
d907062
Compare
/triage accepted |
/lgtm |
LGTM label has been added. Git tree hash: a99dbb28c28b78edd04b037d34b2b150b0db6275
|
/kind cleanup
/priority important-longterm
/sig api-machinery