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

Add a new servlet logging module supporting jakarta.* packages. #153

Merged
merged 19 commits into from
Feb 27, 2023
Merged

Add a new servlet logging module supporting jakarta.* packages. #153

merged 19 commits into from
Feb 27, 2023

Conversation

mofterdinger
Copy link
Contributor

@mofterdinger mofterdinger commented Feb 23, 2023

The latest version 3.6.3 of cf-java-logging-support-servlet is not compatible with the Jakarta Servlet API. The Jakarta Servlet API is also used by the new Spring Framework 6.
This PR adds a new module cf-java-logging-support-servlet-jakarta which supports the jakarta servlet API. This PR copies the source and test-source code of module cf-java-logging-support-servlet to the new module. Then it replaces the packages javax.* with jakarta.* and performs the tests.

@cla-assistant
Copy link

cla-assistant bot commented Feb 23, 2023

CLA assistant check
All committers have signed the CLA.

@mofterdinger
Copy link
Contributor Author

I'm not able to sign the CLA, I get this error:

Error
There is no CLA to sign for SAP/cf-java-logging-support
(Bad credentials)

@KarstenSchnitter
Copy link
Contributor

Thanks, for providing this change. I will clarify, what the problem with the CLA is. Members of the SAP Github organisation should have the CLA signed that way.

Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. The changeset looks pretty well. There are some issues with the code management. I added some questions in the review.

cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
Comment on lines +121 to +124
<groupId>${project.groupId}</groupId>
<artifactId>cf-java-logging-support-servlet</artifactId>
<version>${project.version}</version>
<type>jar</type>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of building and using a jar compared to accessing the folder directly? The code will always be built together, won't it?

Copy link
Contributor Author

@mofterdinger mofterdinger Feb 23, 2023

Choose a reason for hiding this comment

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

The benefit is from my point of view, that the maven-dependency-plugin does the job to provide and extract the source code. We don't have to care about the project structure to find the sources and copy them into this module. I assume this can be also done, but I don't know how to do this with maven.

Comment on lines +87 to +88
<goal>jar</goal>
<goal>test-jar</goal>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this created more artefacts, that will be uploaded to Maven Central. Note, that in the parent pom.xml, there is the "ossrh" profile, that creates, signs and uploads the artefacts. This already contains a maven-source-plugin. Do you have an idea, how these two configurations interact?

Copy link
Contributor Author

@mofterdinger mofterdinger Feb 23, 2023

Choose a reason for hiding this comment

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

You could be right, that could really a problem. If you have look into the target folder of this module, it contains 2 additional jars. Can somehow control which artifacts are uploaded to Maven central? Do you have something like an artifact or module list? To be honest I'm not familiar with this upload.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the publishing. It almost looks fine, but for the jar, that contains the sources. It does not exists, since its execution phase is so early in the Maven lifecycle, that all the actions for providing the correct code come afterwards. I will add the required configuration, once this PR is merged. Without a sources jar, Maven Central would reject this project.

cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
cf-java-logging-support-servlet-jakarta/pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

The build of this change is not quite stable yet. There are reproducible test failures when I run mvn clean install. A subsequent run of mvn install will succeed, but a mvn clean will break the next build.

@Mazhar0910
Copy link

@KarstenSchnitter @mofterdinger , Thanks for considering the support for jakarta.* packages.
I am in a process to migrate our app to Spring 3.x, however we make use of cf java logging libraries.

May be i would like to know by when we can expect this change to be available into SAP Internal Artifactory ?

Regards,
Mazhar

@KarstenSchnitter
Copy link
Contributor

@Mazhar0910 I am currently clarifying the usage of the additional Maven plugin. This is a prerequisite for a merge of this change. It would be very beneficial if the first Maven build after checkout was succeeded, as well. The build issue I could work-around for now, though. I still have to check, what kind of artefacts will be uploaded to Maven Central. Still, I do not think, these issues will take to long to clarify or mitigate. I estimate a new library version containing this change in early March.

@KarstenSchnitter
Copy link
Contributor

Currently mvn test -Dtest=CustomFilterTest is failing, when executed in the new module. I think, it does not pick up the logging configuration in logback-test.xml. The log output does not match the output from the original test in cf-java-logging-support. Since the test validates this output, the test currently fails.

@aamotharald
Copy link
Contributor

Hi colleagues, Hi @mofterdinger ,

please consider my PR:
https://github.com/mofterdinger/cf-java-logging-support/pull/1

It is using a plugin that needs no extra license checking.
Also the wrong logback-test.xml file location got fixed. Tests execute green on my machine ..

other plugin for javax to jakarta
@mofterdinger
Copy link
Contributor Author

@aamotharald Thanks for your PR, with your changes the test no longer fail :-). From my point of view it looks good.

Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks @mofterdinger and @aamotharald for the contribution. This looks fine. I will have to do some minor changes to provide a sources jar for the new Jakarta modules. But this does not need to be part of this PR. Currently CLA validation is failing again. @aamotharald can you check, whether your account is connected with the SAP org. I will recheck the status repeatedly and work on the missing jar.

@KarstenSchnitter KarstenSchnitter merged commit c8f4f2a into SAP:main Feb 27, 2023
@KarstenSchnitter
Copy link
Contributor

Once more, thanks for the contribution. I will go over the dependencies of the entire project and create a new release afterwards.

@mofterdinger mofterdinger deleted the servlet_jakarta branch February 27, 2023 09:32
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.

4 participants