-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
[JEP-222] Add experimental WebSocket support + Jetty update in Winstone + Remoting 4.0 #4369
Conversation
Looks like there are some test failures. |
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.
So far I've just done a quick overview review. It looks good. It's well structured and easy to follow. I like how nicely it fits into the existing structure while providing valuable new capability.
I'll do a more thorough review at a later time.
@jglick what is the rollout plan for this change? Do you want to have it as |
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 haven't tested it, but the code looks good
I do not think there is any formal mechanism for any such marking. We can phrase it as being experimental in the weekly changelog of course. |
Changelog would be enough. APIs are already marked as |
And could probably stay that way unless plugins start to want to implement their own endpoints. |
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.
Haven't tested but code looks good
requesting reviews from the @jenkinsci/core team since it is a major change under the hood of Jenkins. Unfortunately I do not have bandwidth to review it till the next week (wed or so), but I do not mind if it gets merged.with the current approvals. @jglick Could you please also add the "collateral improvements" from Remoting and Winstone to the changelog?
|
I plan to merge it tomorrow if no negative feedback |
Note that I already requested reviews from @jenkinsci/core-pr-reviewers. |
@jglick would you be interested to write a blog post about this new feature? |
Yes I should be able to do that. |
Merged toward 2.216 FTR. |
See JEP-222. Downstream of
jenkinsci/winstone#79+jenkinsci/remoting#357+jenkinsci/jenkins-test-harness#183.Proposed Changelog Entries