Add mvnw and update dependencies #321
Conversation
karianna
commented
Oct 3, 2024
karianna
commented
- Add mvnw
- Updated Build dependencies and plugins, all profiles build, test and run correctly, mvn site now also supported
- Updated core app dependencies, everything passes
- Updated all dependencies as far as they will go without failures
|
These are quite a few changes so quite risky. I have to ask, have you manually tested everything? At this point, you should treat our automated testing as non-existent. What we have is basically just a token example of tests. That means without thorough manual testing, there is a good chance this scale of changes all at once will break something. If you don’t test it, I will need to do it and try to fix any problems that were introduced. |
… workflows to use mvnw
I manually tested the UI for all of the servers/profiles + the csv bulk uploads. Is there other manual testing you would typically do? Happy to do so! |
|
As long as you looked at this: https://github.com/eclipse-ee4j/cargotracker?tab=readme-ov-file#exploring-the-application + this: https://jakarta.ee/learn/docs/cargotracker-documentation/current/gettingstarted/main/main.html and made a good faith effort, that's all I am looking for. After that, I will test myself before I review the PR more closely and merge anyway. |
OK, let me go through that checklist and I'll get back to you! |
|
I've tested on the 3 profiles (payara, glassfish, liberty). I executed every option in the UI that were obvious to me / possible, no errors reported. However, I couldn't properly test the cloud profile, but I guess we'll get to that when we deploy to a new hosting provider? |
|
Yes, you can ignore the cloud profile for now. I'll test as soon as I am able. |
|
When I follow our getting started instructions, I am now getting this: It appears this is because the Maven Enforcer plugin was introduced. Can you kindly explain why this is necessary? We are an open source project and we need make sure we impose as few requirements on our users as possible. Also bear in mind, we support quite "outdated" environments. For example, we fully support Java SE 11 and for a long time we supported Java SE 8. If we enforce a Maven version at all, it needs to be the oldest one possible. I have set the PR to draft for now. Let's iterate to make sure I can merge the changes as soon as possible. Thanks! |
|
I typically add this as a std thing for encouraging a security baseline as well as reproducible builds, but I take your point! I've pushed an update that comments out the enforcer so it no longer runs. |
|
OK. Unfortunately have other commitments now but I will resume review during the weekday evenings if time permits. |
|
Everything still works. That's great! |
mvnw
Outdated
There was a problem hiding this comment.
This is a great addition. Can you please also update all the READMEs, website, and docs, replacing the installed Maven references?
There was a problem hiding this comment.
I've updated the README, but not sure where the website and docs code lives?
pom.xml
Outdated
| <artifactId>maven-deploy-plugin</artifactId> | ||
| <version>${maven.deploy-plugin.version}</version> | ||
| </plugin> | ||
| <!-- Uncomment this and the enforcer plugin section in the plugins section below if you want to enforce a particular version of Maven --> |
There was a problem hiding this comment.
Can we please just remove this? We have a lot of dead code in the project already. If we ever decide to go this route, we will add it back in properly? Alternatively, it's not a bad idea enforcing a Maven version. It's just that it can't be any newer than really necessary.
pom.xml
Outdated
| @@ -336,36 +751,39 @@ | |||
| </plugins> | |||
| </build> | |||
| </profile> | |||
| <!-- Cloud/production deployment using Payara and PostgreSQL. --> | |||
| <!-- Cloud/production deployment using Payara and PostgresSQL. --> | |||
There was a problem hiding this comment.
PostgreSQL is correct, no: https://en.wikipedia.org/wiki/PostgreSQL?
| @@ -19,10 +19,48 @@ | |||
| <url>https://github.com/eclipse-ee4j/cargotracker/issues</url> | |||
| </issueManagement> | |||
| <properties> | |||
| <!-- TODO Moving to 1.8.1.Final+ causes glassfish profile to fail --> | |||
There was a problem hiding this comment.
Are we being consistent about using Maven properties? I typically only use it for very prominent dependencies like Jakarta EE or if something is used multiple times. Otherwise I just leave it embedded (I actually find it easier to maintain that way). Any sensible scheme is fine, as long as we are being conscious and consistent. Can you please give it some more thought and adjust accordingly?
There was a problem hiding this comment.
I've updated this section and added a comment as to how the properties are being used and made the rest consistent. LMK if you want a different approach though!
m-reza-rahman
changed the title
