FINERACT-797: Use Spring Boot BOM to avoid maintaining version numbers in dependencies.gradle #662

xurror · 2019-11-30T02:40:54Z

FINERACT-797 - Spring Dependency Management

Update mockito to latest version
Move dependencies from dependencies.gradle to build.gradle
Removed implemented TODOs from XBRBuilderTest.java
https://issues.apache.org/jira/browse/FINERACT-797

Description

In an ideal world, in a future PR, it would be really really cool if we could just avoid having to repeat using fixed version, and could use the implicit versions via Spring BOM dependency management, see https://docs.spring.io/spring-boot/docs/current/gradle-plugin/reference/html/#managing-dependencies ...

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
API documentation at https://github.com/apache/fineract/blob/develop/api-docs/apiLive.htm has been updated with details of any API changes.
Integration tests have been created/updated for verifying the changes made.
All Integrations tests are passing with the new commits.
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

xurror · 2019-11-30T02:48:13Z

@vorburger please can you review this?

vorburger · 2019-12-01T02:03:14Z

@ivan333m do you have any interest in helping to review this, given that this idea originally came up when I reviewed your PR #642? (I'll go over it as well and post some feedback from my side ASAP.)

vorburger

Some minor feedback, and a question:

Would you be able to post a diff of a before/after dependency tree, so that we can see what versions, if any, will effectively change with this PR?

fineract-provider/build.gradle

vorburger · 2019-12-01T02:07:13Z

fineract-provider/build.gradle

+  imports {
+    mavenBom 'org.springframework:spring-framework-bom:5.1.10.RELEASE'
+  }
+  dependencies {


I'm probably misunderstanding the Spring BOM, but does it not also define (some of) these versions? Or is this list just the versions of all Fineract dependencies which the Spring BOM does not fix?

Well, the spring framework-bom resolves all spring artifacts(like spring-core etc) without explicitly declaring them since they all use thesame version

It seems there is no spring-boot-bom to handle the spring boot artifacts and dependencies

My primary idea was to use generalized bom that handles dependencies for most of the packages we use and even more but the version conflicts were too big breaking builds

but the version conflicts were too big breaking builds

@xurror OK, then let's do that (you or someone else later) separately; I've just created new https://issues.apache.org/jira/browse/FINERACT-805 so that we don't forget to follow-up on that... (It's surely best to raise small PRs for each one of these, instead of one big one for everything together.)

fineract-provider/src/test/java/org/apache/fineract/mix/report/service/XBRLBuilderTest.java

ivbond · 2019-12-01T07:01:18Z

@xurror Nice changes, thanks.

All points described bellow are OPTIONAL!

I would propose following:

According to the spring doc spring doc
When you apply the io.spring.dependency-management plugin, Spring Boot’s plugin will automatically import the spring-boot-dependencies bom from the version of Spring Boot that you are using..... So you don't need to specify version on spring boot dependencySet
As I know when you apply bom it controls all third-party dependencies that declared it the bom. Here is a spring boot bom. You can find spring and all related third-party dependencies there. Therefore we could exclude spring framework version and alot of third-party.

You could generate spring boot application on Spring IO initializer and use it as an example.

xurror · 2019-12-01T22:01:07Z

@xurror Nice changes, thanks.

All points described bellow are OPTIONAL!

I would propose following:

According to the spring doc spring doc
When you apply the io.spring.dependency-management plugin, Spring Boot’s plugin will automatically import the spring-boot-dependencies bom from the version of Spring Boot that you are using..... So you don't need to specify version on spring boot dependencySet

As I know when you apply bom it controls all third-party dependencies that declared it the bom. Here is a spring boot bom. You can find spring and all related third-party dependencies there. Therefore we could exclude spring framework version and alot of third-party.

You could generate spring boot application on Spring IO initializer and use it as an example.

Thanks for this review, the new updates I have made should fix that

vorburger · 2019-12-02T01:11:37Z

@xurror this now has conflicts, because I just merged #661 ... sorry about that. Would you mind to (manually...) resolve these conflicts? There's really no way to avoid this, so I usually just merge cases like this in the order that such conflicting changes are proposed (e.g. in this csae, @percyashu #661 was before your #662 here). Thank You!

xurror · 2019-12-02T04:48:19Z

Resolved conflicts with #661

vorburger · 2019-12-02T06:03:03Z

fineract-provider/src/test/java/org/apache/fineract/mix/report/service/XBRLBuilderTest.java

@@ -79,10 +79,13 @@ public void shouldCorrectlyBuildMap() {
            nodes = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new ByteArrayInputStream(result.getBytes()))
                    .getElementsByTagName("Assets");
        } catch (final SAXException e) {
+            // TODO Auto-generated catch block


@xurror this looks weird and given (your) #663 I'm assuming is a mistake - remove?

Suggested change

// TODO Auto-generated catch block

vorburger · 2019-12-02T06:03:16Z

fineract-provider/src/test/java/org/apache/fineract/mix/report/service/XBRLBuilderTest.java

            e.printStackTrace();
        } catch (final IOException e) {
+            // TODO Auto-generated catch block


Suggested change

// TODO Auto-generated catch block

vorburger · 2019-12-02T06:03:24Z

fineract-provider/src/test/java/org/apache/fineract/mix/report/service/XBRLBuilderTest.java

            e.printStackTrace();
        } catch (final ParserConfigurationException e) {
+            // TODO Auto-generated catch block


Suggested change

// TODO Auto-generated catch block

vorburger · 2019-12-02T06:11:26Z

fineract-provider/build.gradle

+     dependency 'com.squareup.okhttp:okhttp:2.0.0'
+     dependency 'com.squareup.okhttp:okhttp-urlconnection:2.0.0'


see https://issues.apache.org/jira/browse/FINERACT-804 ...

vorburger

This would LGTM for me if you could just remove those re-introduced TODOs in XBRLBuilderTest.java? Then I'm happy to merge this. Note to myself, or anyone else (e.g. @awasum) who will merge this: We should "squash" the commits here into 1 via that GitHub UI drop-down merge button.

reverted removed TODOs from XBRLBuilder

awasum requested a review from vorburger November 30, 2019 08:20

vorburger requested changes Dec 1, 2019

View reviewed changes

xurror force-pushed the FINERACT-797 branch from 7f7cd56 to 0803532 Compare December 2, 2019 04:45

vorburger reviewed Dec 2, 2019

View reviewed changes

vorburger requested changes Dec 2, 2019

View reviewed changes

vorburger approved these changes Dec 2, 2019

View reviewed changes

vorburger merged commit a20f76d into apache:develop Dec 2, 2019

xurror added 5 commits December 3, 2019 05:08

resolved

108b65e

remove commented version declarations

ba84ace

Removed dependencySet for org.springframework.boot

2a39591

reverted removed TODOs from XBRLBuilder

resolve conflicts with apache#661

0803532

remove TODOs in XBRLBuilder

baae074

xurror mentioned this pull request May 7, 2020

FINERACT-846: Migrating to Java 11 #820

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-797: Use Spring Boot BOM to avoid maintaining version numbers in dependencies.gradle #662

FINERACT-797: Use Spring Boot BOM to avoid maintaining version numbers in dependencies.gradle #662

xurror commented Nov 30, 2019 •

edited

xurror commented Nov 30, 2019

vorburger commented Dec 1, 2019

vorburger left a comment

vorburger Dec 1, 2019

xurror Dec 1, 2019

xurror Dec 1, 2019

vorburger Dec 2, 2019

ivbond commented Dec 1, 2019

xurror commented Dec 1, 2019

vorburger commented Dec 2, 2019

xurror commented Dec 2, 2019

vorburger Dec 2, 2019

vorburger Dec 2, 2019

vorburger Dec 2, 2019

vorburger Dec 2, 2019

vorburger left a comment

		dependency 'com.squareup.okhttp:okhttp:2.0.0'
		dependency 'com.squareup.okhttp:okhttp-urlconnection:2.0.0'

FINERACT-797: Use Spring Boot BOM to avoid maintaining version numbers in dependencies.gradle #662

FINERACT-797: Use Spring Boot BOM to avoid maintaining version numbers in dependencies.gradle #662

Conversation

xurror commented Nov 30, 2019 • edited

Description

Checklist

xurror commented Nov 30, 2019

vorburger commented Dec 1, 2019

vorburger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivbond commented Dec 1, 2019

xurror commented Dec 1, 2019

vorburger commented Dec 2, 2019

xurror commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorburger left a comment

Choose a reason for hiding this comment

xurror commented Nov 30, 2019 •

edited