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.

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

Adding jakarta servlet API 6 compatibility #247

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danigiri
Copy link

I am trying to use the proxy with jetty 12 ee10, which uses jakarta 6. This change and version bump uses v6
(context https://en.wikipedia.org/wiki/Jakarta_EE).

All tests pass and package is generated

➜  HTTP-Proxy-Servlet : master ✘ :✹ ᐅ  mvn clean compile test package
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  30.635 s
[INFO] Finished at: 2024-04-19T16:48:47+01:00
[INFO] ------------------------------------------------------------------------

Copy link
Collaborator

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

If the servlet is completely unchanged by this dependency upgrade, why bump the major version number to 3.0?

@danigiri
Copy link
Author

@dsmiley I had the thought that a key dependency version bump to a major number warranted a major version change as well to signal it clearly, but there is no other reason, so if it's more aligned we could just bump a minor

@dsmiley
Copy link
Collaborator

dsmiley commented Apr 25, 2024

Let's continue to test that the previous version works as well, since it is a key dependency.
https://github.com/mitre/HTTP-Proxy-Servlet/blob/master/.github/workflows/maven.yml shows versions are specified for Apache HttpClient; same could be done here and likewise with alterations in pom.xml.

@danigiri
Copy link
Author

Added 5 and 6 to the compatibility testing matrix, tested both manually locally and tests are happy as expected. Went for a closed list of options compatibility ([]) as not expecting jakarta versions to change minor numbers that much if at all.

Also moved back to 2.2 as versioning bump, would that be more coherent?

We're leaving it open in the pom and testing with both (and the rest of combinations on the matrix), so we're basically saying that you can use this component with either of the two as a dependency. AFAIU.

@danigiri danigiri changed the title Adding jakarta servlet API 6 Adding jakarta servlet API 6 compatibility Apr 25, 2024
@danigiri
Copy link
Author

Looks like there is a format issue with the yaml, looking into it

@danigiri
Copy link
Author

Error: The template is not valid. .github/workflows/maven.yml (Line: 31, Col: 28): A sequence was not expected

Having a deeper read of https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

Looks like include expands on the matrix, but those are single values only, moved the multiple values to the matrix values and not the include

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants