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

7054 payara bom #7064

Merged
merged 2 commits into from
Jul 13, 2020
Merged

7054 payara bom #7064

merged 2 commits into from
Jul 13, 2020

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Jul 8, 2020

What this PR does / why we need it:
A lot of libraries used by us (and again by those libs) are provided by the Payara application server.

There is no need to bundle those in our WAR (making it smaller) and it's always a good idea to keep things inline with each other, to avoid conflicts between libraries. The newly released Payara BOM is making this significantly easier for basic libraries as Jackson, Jakarta, etc.

Also, our application server maintains a few patched versions of libraries. A good example is the JSF patch recently removed (see #7036), now being bundled with the upstream installation. This change to pom.xml includes the repository of these patched versions, so we use them locally and during tests, too.

Which issue(s) this PR closes:

Closes #7054

Special notes for your reviewer:
The Jackson BOM has been removed due to the fact that the Payara BOM already provides that one.

Suggestions on how to test this:
Nada. Usual things. I didn't move much (the deps cleanup is still to come), so this should not have a bigger impact.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nada.

Is there a release notes update needed for this change?:
Nada.

Additional documentation:
https://blog.payara.fish/align-dependencies-with-payara-bom

@coveralls
Copy link

Coverage Status

Coverage remained the same at 19.653% when pulling b584a06 on poikilotherm:7054-payara-bom into 6daf219 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I read the blog post linked from the issue and I think I understand the goal but it seems like this is more of a clean up effort than solving a problem we're having (classloading conflicts or whatever).

For now I left a few questions. I'm fine with someone else moving this to QA since I don't feel especially passionate or knowledgeable about all this dependency management.

@@ -26,11 +26,8 @@
<skipUnitTests>false</skipUnitTests>

<jakartaee-api.version>8.0.0</jakartaee-api.version>
<!-- Make this correspond to the version used in Payara: 5.201 uses (patched) 2.3.14 -->
<mojarra.version>2.3.14</mojarra.version>
<payara.version>5.2020.2</payara.version>
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that this new "payara.version" property needs to be bumped every time we want to move to a new version of Payara?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be moved. This is not a must, but a strong should.

This hasn't happened in the past with GF 4.1, as that appserver hadn't any updates. It won't be much of a hassle to do this and it makes testing with newer appserver versions easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, as the article outlined, too: you would do this with the Wildfly/Jetty/Liberty BOM when using those appservers.

You might think about creating different maven profiles when multi appserver compatibility becomes a feature.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Maybe after we've done a few of these Payara upgrades (pull request #7036 was the last one), we'll get into a nice pattern of which files need to be touched (similar to how we know which files to touch when making a release, and even document them). So heads up here to @donsizemore (who made the last bump) that a new line is being added.

pom.xml Outdated
@@ -90,24 +87,35 @@
<name>Local repository for hosting jars not available from network repositories.</name>
<url>file://${project.basedir}/local_lib</url>
</repository>
<repository>
<id>payara-patched-externals</id>
Copy link
Member

Choose a reason for hiding this comment

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

Does this further tie us to Payara? Will it be harder to move to Liberty or Wildfly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But we are pretty bound to Pasta anyway, as we rely on their patched lib releases. As long as those don't hit upstream, it would lead to troubles when deploying to Wildfly/Liberty/... anyway.

@@ -63,6 +60,17 @@
<!--Maven checks for dependendies from these repos in the order shown in the pom.xml
This isn't well documented and seems to change between maven versions -MAD 4.9.4 -->
<repositories>
<repository>
<id>payara-patched-externals</id>
Copy link
Member

Choose a reason for hiding this comment

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

Does this further tie us to Payara? Would it be harder to switch to Liberty or Wildfly, for example?

Copy link
Member

Choose a reason for hiding this comment

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

Whoops. I asked this twice. Here's the answer from @poikilotherm from #7064 (comment)

"Yes. But we are pretty bound to Payara anyway, as we rely on their patched lib releases. As long as those don't hit upstream, it would lead to troubles when deploying to Wildfly/Liberty/... anyway."

@@ -26,11 +26,8 @@
<skipUnitTests>false</skipUnitTests>

<jakartaee-api.version>8.0.0</jakartaee-api.version>
<!-- Make this correspond to the version used in Payara: 5.201 uses (patched) 2.3.14 -->
<mojarra.version>2.3.14</mojarra.version>
Copy link
Member

Choose a reason for hiding this comment

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

Does the version of Mojarra no longer matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but it is already included in the BOM. It should be kept the same as the appserver anyway, so that updating is now obsolete.

Instead, we change the appserver version in payara.version.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

At this point @poikilotherm has answered all my questions so I'm moving this to QA. I haven't done any testing but the changes make sense conceptually to me. Rather than specifying the version of Mojarra with a comment indicating it should be in sync with the version from Payara, we specify the version of Payara. Seems like a win to me.

@kcondon kcondon self-assigned this Jul 13, 2020
@kcondon kcondon merged commit 4f8038a into IQSS:develop Jul 13, 2020
@djbrooke djbrooke added this to the Dataverse 5 milestone Jul 14, 2020
@poikilotherm poikilotherm deleted the 7054-payara-bom branch August 10, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency management: start using Payara BOM
5 participants