Skip to content

Conversation

@edddddy
Copy link
Contributor

@edddddy edddddy commented Feb 7, 2023

Why are the changes needed?

As mentioned in #4247, it seems to be better that using the same maven version with master. Thus, modify the version check logic in build/mvn from equal or greater to equal.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@pan3793
Copy link
Member

pan3793 commented Feb 7, 2023

Hi @edddddy, thanks for your contribution, would you please fill out the PR template, and supply more background?

@codecov-commenter
Copy link

Codecov Report

Merging #4261 (11f19ec) into master (1b92d80) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4261      +/-   ##
============================================
- Coverage     53.41%   53.38%   -0.03%     
  Complexity       13       13              
============================================
  Files           560      560              
  Lines         30562    30562              
  Branches       4139     4139              
============================================
- Hits          16324    16316       -8     
- Misses        12705    12709       +4     
- Partials       1533     1537       +4     
Impacted Files Coverage Δ
.../apache/kyuubi/server/api/v1/BatchesResource.scala 70.09% <0.00%> (-2.81%) ⬇️
...ache/kyuubi/operation/KyuubiOperationManager.scala 80.00% <0.00%> (-2.67%) ⬇️
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala 69.02% <0.00%> (-0.55%) ⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 97.44% <0.00%> (-0.07%) ⬇️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala 79.01% <0.00%> (+0.61%) ⬆️
...apache/kyuubi/engine/JpsApplicationOperation.scala 80.64% <0.00%> (+3.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pan3793 pan3793 changed the title [KYUUBI #4247]change the if condition to equal [KYUUBI #4247] build/mvn should use the exact version defined in pom.xml Feb 7, 2023
@pan3793 pan3793 modified the milestones: v1.7.0, v1.6.2 Feb 7, 2023
@pan3793 pan3793 closed this in 95318d5 Feb 7, 2023
pan3793 pushed a commit that referenced this pull request Feb 7, 2023
…om.xml`

### _Why are the changes needed?_

As mentioned in #4247, it seems to be better that using the same maven version with master. Thus, modify the version check logic in `build/mvn` from equal or greater to equal.

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4261 from edddddy/mvn_ver_cp.

Closes #4247

11f19ec [1456173400] change the if condition to equal

Lead-authored-by: edddddy <rmrfall@qq.com>
Co-authored-by: 1456173400 <1456173400@qq.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 95318d5)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Feb 7, 2023

Thanks, merged to master/1.6

pan3793 added a commit that referenced this pull request Feb 9, 2023
### _Why are the changes needed?_

`build/mvn` should download the maven when the local `mvn` version does not match, this corrects the change in #4261

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4291 from pan3793/mvn-version.

Closes #4247

42717a7 [Cheng Pan] [KYUUBI #4247][FOLLOWUP] Fix build/mvn version comparison

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 added a commit that referenced this pull request Feb 9, 2023
### _Why are the changes needed?_

`build/mvn` should download the maven when the local `mvn` version does not match, this corrects the change in #4261

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4291 from pan3793/mvn-version.

Closes #4247

42717a7 [Cheng Pan] [KYUUBI #4247][FOLLOWUP] Fix build/mvn version comparison

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
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.

4 participants