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

[MINOR] Adopt maven wrapper to use the same version of Maven #4242

Closed
wants to merge 3 commits into from

Conversation

jongyoul
Copy link
Member

@jongyoul jongyoul commented Oct 5, 2021

What is this PR for?

Using maven wrapper when building and testing

What type of PR is it?

[Improvement]

Todos

  • - Change mvn to ./mvnw

What is the Jira issue?

N/A

How should this be tested?

Pass CI

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@Reamer
Copy link
Contributor

Reamer commented Oct 5, 2021

I've heard of the Maven wrapper, but didn't know what the project was for until today. I read the following article about it.
https://www.baeldung.com/maven-wrapper
I am against using the maven wrapper for the following reasons.

  1. A Jar is stored in the SCM.
  2. I see it as an advantage to have multiple versions in the wild to build the zeppelin project.

@jongyoul
Copy link
Member Author

jongyoul commented Oct 5, 2021

A Jar is stored in the SCM.

This is a very similar mechanism with the Gradle wrapper which stores only one jar(50KB) is to get the actual Gradle version only. As you already knew, the jar is not a maven itself. It's just a wrapper program to get a specific version of Maven when running maven wrapper for the first time.

I see it as an advantage to have multiple versions in the wild to build the zeppelin project.

I don't think Maven is not unstable at all but the main advantage of using it is to keep the same version for CI and the development environment. If we want to multiple versions of maven as we have multiple projects using maven, it's quite complicated. If we use a maven wrapper, we don't care about the maven version for Zeppelin because maven wrapper download and use the specific version for Zeppelin project. Moreover, we don't have to install maven in my local machine to build Zeppelin - yes, I also think most Java developers already installed Maven :-) -

The most important thing is that it's not mandatory to use a maven wrapper. It's just a helper feature. Everyone doesn't have to use a maven wrapper. If someone wants to use local maven, it's ok to use it as it has the same behaviors. Nonetheless, the reason why I thought I would like to adopt it is that I just want to use the same version of CI. (Recently, I got different warning messages between CI and my local machine.)

@jongyoul
Copy link
Member Author

jongyoul commented Oct 7, 2021

I would like to merge it if there isn't any further opinion. I believe that some people use this feature very usefully.

Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

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

LGTM

@jongyoul jongyoul closed this in 26162dc Oct 7, 2021
@zjffdu
Copy link
Contributor

zjffdu commented Oct 7, 2021

@jongyoul This is not a small change, it's better to create a ticket for it.

zlosim pushed a commit to zlosim/zeppelin that referenced this pull request Oct 22, 2021
Using maven wrapper when building and testing

[Improvement]

* [ ] - Change `mvn` to `./mvnw`

N/A

Pass CI

* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#4242 from jongyoul/minor/add-mvnw and squashes the following commits:

a606683 [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
d5e9bbb [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
75300ce [Jongyoul Lee] [MINOR] Add mvnw in order to use the same version of maven
zlosim pushed a commit to zlosim/zeppelin that referenced this pull request Oct 22, 2021
Using maven wrapper when building and testing

[Improvement]

* [ ] - Change `mvn` to `./mvnw`

N/A

Pass CI

* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#4242 from jongyoul/minor/add-mvnw and squashes the following commits:

a606683 [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
d5e9bbb [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
75300ce [Jongyoul Lee] [MINOR] Add mvnw in order to use the same version of maven
JasonLeeCoding pushed a commit to JasonLeeCoding/zeppelin that referenced this pull request Jan 5, 2022
### What is this PR for?
Using maven wrapper when building and testing

### What type of PR is it?
[Improvement]

### Todos
* [ ] - Change `mvn` to `./mvnw`

### What is the Jira issue?
N/A

### How should this be tested?
Pass CI

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jongyoul Lee <jongyoul@gmail.com>

Closes apache#4242 from jongyoul/minor/add-mvnw and squashes the following commits:

a606683 [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
d5e9bbb [Jongyoul Lee] [MINOR] adopt maven wrapper to use the same version of Maven
75300ce [Jongyoul Lee] [MINOR] Add mvnw in order to use the same version of maven
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants