Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FLS] Complete migration to Jetty-12 and consider handlers again #1258

Merged
merged 1 commit into from
Dec 16, 2023
Merged

[FLS] Complete migration to Jetty-12 and consider handlers again #1258

merged 1 commit into from
Dec 16, 2023

Conversation

HannesWell
Copy link
Contributor

This completes the migration of Passage's JettyHandler and JettyRequest to Jetty-12,which was started in #1253, but has left the FLS in a dysfunctional state.
This PR completes that migration so that response handlers are considered again to restore the FLS full functionality.

The changes are done with the great help of the Jetty-12 migration guide:
https://eclipse.dev/jetty/documentation/jetty-12/programming-guide/index.html#pg-migration-11-to-12-servlet-to-handler

@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023 •
edited
Loading

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (12031b8) 33.45% compared to head (377a387) 33.48%.

Files Patch % Lines
...lipse/passage/lic/internal/jetty/JettyHandler.java 0.00% 15 Missing ⚠️
...lipse/passage/lic/internal/jetty/JettyRequest.java 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1258      +/-   ##
============================================
+ Coverage     33.45%   33.48%   +0.02%     
  Complexity      359      359              
============================================
  Files          1175     1175              
  Lines         25763    25743      -20     
  Branches       1594     1589       -5     
============================================
  Hits           8619     8619              
+ Misses        16623    16603      -20     
  Partials        521      521

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eparovyshnaya
eparovyshnaya requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @HannesWell

Thank you a lot for finding resources to complete FLS codebase migration to new Jetty API.

I'd just ask you to pay a bit of attention to the project code guidelines, which, for instance, prohibit compound names for vars and fields. And to the rest of my comments.

Thank you again for your time.

...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyRequest.java Outdated Show resolved Hide resolved
@HannesWell HannesWell force-pushed the complete-fls-to-jetty12-migration branch from fcb5f8c to 9f37a3c Compare December 13, 2023 15:41
@HannesWell
Copy link
Contributor Author

Thank you @eparovyshnaya for the fast review.

I tried to apply all your specific and general remarks. Please let me know if I missed something.

eparovyshnaya
eparovyshnaya requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've just missed one compound name.

Otherwise - perfect work.

...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
@HannesWell HannesWell force-pushed the complete-fls-to-jetty12-migration branch from 9f37a3c to 4c1316e Compare December 14, 2023 11:11
eparovyshnaya
eparovyshnaya requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed compound parameter name.

...s/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyHandler.java Outdated Show resolved Hide resolved
@HannesWell
[FLS] Complete migration to Jetty-12 and consider handlers again
377a387 
This completes the migration of Passage's JettyHandler and JettyRequest
to Jetty-12 so that response handlers are considered again and the FLS
works again.

Jetty-12 migration guide
https://eclipse.dev/jetty/documentation/jetty-12/programming-guide/index.html#pg-migration-11-to-12-servlet-to-handler
@HannesWell HannesWell force-pushed the complete-fls-to-jetty12-migration branch from 4c1316e to 377a387 Compare December 15, 2023 16:45
eparovyshnaya
eparovyshnaya approved these changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @HannesWell

@eparovyshnaya eparovyshnaya merged commit 679fc23 into eclipse-passage:master Dec 16, 2023
5 of 6 checks passed
@eparovyshnaya
Copy link
Contributor

@ruspl-afed
can we maybe issue a patch-release for this critial bug which is fixed by this PR (many thanks to that @HannesWell)?

@HannesWell HannesWell deleted the complete-fls-to-jetty12-migration branch December 16, 2023 07:50
@HannesWell
Copy link
Contributor Author

@ruspl-afed can we maybe issue a patch-release for this critial bug which is fixed by this PR (many thanks to that @HannesWell)?

Your welcome.
A patch release would be nice to have. If you have a quickly to implement idea for #1260 it would be great to include that as well. :)

@eparovyshnaya eparovyshnaya mentioned this pull request Dec 16, 2023
Closed
@ruspl-afed
Copy link
Contributor

@ruspl-afed can we maybe issue a patch-release for this critial bug which is fixed by this PR (many thanks to that @HannesWell)?

Release for Passage 2.10.1 has been planned.
Many thanks to @HannesWell for this fix!

@ruspl-afed ruspl-afed added this to the 2.10.1 milestone Dec 16, 2023
HannesWell added a commit to HannesWell/eclipse.passage that referenced this pull request Apr 29, 2024
@HannesWell
[FLS] Fix completion of responses without payload
7fb8b18 
This was forgotten in eclipse-passage#1258.
@HannesWell HannesWell mentioned this pull request Apr 29, 2024
Merged
eparovyshnaya pushed a commit that referenced this pull request Apr 29, 2024
@HannesWell @eparovyshnaya
[FLS] Fix completion of responses without payload
1bc71e5 
This was forgotten in #1258.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eparovyshnaya eparovyshnaya eparovyshnaya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10.1
Development

Successfully merging this pull request may close these issues.
3 participants
@HannesWell @eparovyshnaya @ruspl-afed