-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use Maven Wrapper to build #1548
Conversation
And do not rely on third party tools as they may do things that surprise us.
or we can add:
|
There is a setup-maven action: I use it all the time without any issues and you can perfectly control the maven version. |
We use that action, and the last IT failures made us realize how strange it behaves: it set up different Maven versions per OS:
True, we did not ask for specific maven version, but that is not the point. We should use wrapper anyway to be able to precisely control which Maven version we want to use (across all OSes) but also for dogfooding purposes. See |
Well I would call it strange to use an action without specify a version ... I mean even the readme tells you to specify one, i more suspect that without a version nothing is done actually.... |
I can't see the use of this action for ITs. We do use |
Maybe it is confused with setup-java having support for maven toolchains and caching option named maven ... If one wants there is a composite action https://github.com/marketplace/actions/setup-maven-action but I usually found setup-java/maven enough and more flexible. |
setup-java uses OS maven version which is likely not what we want. |
setup-java has nothing to do with installing / "using" maven it just do the following:
beside that I hope that the itest are using the maven that was build anyways (so a matrix des not make much sense here), so whats left is that the maven version to run the build is better specified explicitly to prevent any surprise. |
You are mixing things. Again, we do NOT use |
IMHO we should not use any action to setup Maven but wrapper.... that's my 5 cents (we even have examples of "maven matrix" that is simply doable with wrapper). |
I mentioned you need to use setup-maven action, you replied that
so I obviously do not mix up anything here but maybe you have just misread my reply... |
Actually, we're not using it... I thought, but the matrix is used with wrapper So I'm fine using wrapper here too. |
@@ -47,7 47,7 @@ jobs: | |||
cache: 'maven' | |||
|
|||
- name: Build with Maven | |||
run: mvn verify -e -B -V -DdistributionFileName=apache-maven | |||
run: ./mvnw verify -e -B -V -DdistributionFileName=apache-maven |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before this step we can also add step Set up Maven
with wrapper
and we not need add wrapper properties and scripts to code
Superseded by #1550 |
#1550 is for master ... this one was for 3.x branch |
I will create a new one for 3.x 😄 |
Done in #1553 |
And do not rely on third party tools as they
may do things that surprise us.