FINERACT-1188: README and other files are not covered by Spotless #1396

vidakovic · 2020-10-13T02:30:36Z

Description

This is a replacement for #1381 for FINERACT-1188

Checklist

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

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

vidakovic · 2020-10-13T02:34:10Z

@ptuomola FYI: I even got the Eclipse launcher configuration fixed; when you import the project into your workspace it immediately starts a build. Bonus: the symbolic link to gradlew is also not necessary anymore (removed it).

ptuomola · 2020-10-13T05:22:42Z

@vidakovic looks like the integration tests are failing... did they work for you locally?

ptuomola · 2020-10-13T05:28:54Z

@vidakovic unfortunately this has broken Eclipse for me again - we are back at not finding Gradle:

Errors occurred during the build.
Errors running builder 'Integrated External Tool Builder' on project 'fineract-provider'.
Variable references non-existent resource : ${workspace_loc:/fineract/gradlew}
Variable references non-existent resource : ${workspace_loc:/fineract/gradlew}

If this works for you, are you setting up your Eclipse workspace based on the instructions in the README ie section "Instructions to run and debug in Eclipse IDE"? Or are you doing something else?

vidakovic · 2020-10-13T08:49:00Z

@ptuomola I have non of the errors you describe. Just to show you below a couple of screenshots from my Eclipse workspace:

... the error log shows one error entry, but related to the language server plugin; I think not related to anything we do:

... and this is the console output from the build that immediately starts after importing the project:

... the package explorer view looks also clean (no error indicators):

vidakovic · 2020-10-13T08:51:53Z

... (continued): I didn't run the integration tests locally (was a bit late already); what I did do is build the Docker image and run it; no problems there. I will checkout what happens with the integration tests; I see still no reason why they should fail. I was not aware of a specific Eclipse setup; will try to recreate your environment according to README instructions... maybe I find the issue like that. Will keep you posted.

Update:

@ptuomola concerning Eclipse: I know now what's going on... I didn't execute "./gradlew cleanEclipse eclipse", because this is actually not necessary. If you import the project like this:

... then everything works perfectly well. From what I see we could get rid of the Eclipse Gradle plugin; even the Open JPA launcher configuration is automatically included in the project without any additional "./gradlew cleanEclipse eclipse". Unless there is another reason for this? Please let me know... I'll update the README if we can drop it.

vidakovic · 2020-10-13T10:31:08Z

Ok... one step ahead: finally have a candidate for the failing integration tests. I did an experiment with a locally running Fineract Docker container that had all database details setup correctly. I've disabled Carge in build.gradle temporarily and ran the tests again... voila it worked. I think what happens with Cargo: it doesn't find the deployable WAR file; it doesn't seem that Fineract is started with Cargo at all.

vidakovic · 2020-10-13T10:55:57Z

@ptuomola yup, it was the Cargo plugin. The problem was that for some reason it didn't detect the WAR file anymore... so I added a deployable section. Just ran the integration tests locally, no failures. Let's see how the Travis build goes to confirm. If it's successful would be great if you could have another look at this. Please let me know if my notes on Eclipse are OK for you; the "new" instructions are actually shorter than the current README and from what I see works at least as well as before.

ptuomola · 2020-10-13T10:59:34Z

@vidakovic Ah I see - you're importing this as an Gradle project rather than Eclipse project.

But if you do that, do you get any of the benefits of Eclipse - e.g. debugging, hot code replace, automatic incremental recompile etc?

vidakovic · 2020-10-13T11:01:35Z

@ptuomola as far as I can see all the same

ptuomola · 2020-10-13T12:23:34Z

@vidakovic cool - just tried this and yes, it seems to work equally well. So no issue from Eclipse perspective.

But the build still seems to be failing? Think there's some problem with the deployment to Cargo:

No such property: war for class: java.lang.String

vidakovic · 2020-10-13T12:33:46Z

@ptuomola this Cargo plugin is a gift that just keeps giving... let me see

vidakovic · 2020-10-13T12:38:28Z

Problem was the use of "." right after a variable in a Groovy string; fixed

vidakovic · 2020-10-13T14:37:19Z

... of course there's something else... but it's something in the Travis configuration... all the tests etc. went through without a problem. Looking into this.

.travis.yml

build.gradle

fineract-provider/build.gradle

vidakovic · 2020-10-13T18:34:27Z

@ptuomola please see my replies; I removed the Eclipse Gradle plugin references and updated the README

build.gradle

vidakovic · 2020-10-13T19:54:11Z

@ptuomola Spotless complains a format issue in README line 200, but when I run locally it doesn't. Any ideas? Cannot reproduce. Could this be coming from another commit upstream?

Update: I've merged the upstream changes. The interesting part was only the new ActiveMQ section in the README... which had a forbidden space at the end of the title.

vidakovic · 2020-10-13T22:42:38Z

@vorburger @ptuomola wow... please let's merge before something else happens upstream ;-)

vorburger · 2020-10-13T23:18:31Z

@vidakovic Whoa! Nice. I'll review ASAP, but 1 quick minor practical feedback already: If you change the usual merge drop-down (above) to the Rebase and merge which we typically prefer using (see https://github.com/apache/fineract#merge-strategy), you'll see that GitHub shows This branch cannot be rebased due to conflicts. That means we would HAVE to merge your 2 commits here with Create a merge commit, which is possible, but always a bit "ugly" and we try to avoid, or with Squash and merge and that creates weird author/ownership metadata (check out e.g. latest e57b7da). If you don't mind, just do a git rebase develop (fixing anything required), and git push --force vidakovic, and it will be a little bit "cleaner". With that out of the way, I'll review the actual changes now! 😈

vorburger · 2020-10-13T23:20:47Z

README.md

-2. Import the fineract-provider project into your Eclipse workspace (File->Import->General->Existing Projects into Workspace, choose root directory fineract/fineract-provider)
-3. Do a clean build of the project in Eclipse (Project->Clean...)
+1. Import the fineract-provider project into your Eclipse workspace (File->Import->Gradle->Existing Gradle Project into Workspace, choose root directory fineract/fineract-provider)
+2. Do a clean build of the project in Eclipse (Project->Clean...)


@vidakovic thanks for this updae, @ptuomola FYI this is actually how I use it as well (without ./gradlew cleanEclipse eclipse)

vorburger · 2020-10-13T23:23:48Z

build.gradle

+gradle clean bootRun'''
+
+project.ext.jerseyVersion = '1.19.4'
+ext['groovy.version'] = '3.0.6'


FTR, the ext['groovy.version'] in fineract-provider/build.gradle used to be 3.0.3, and this upgrades it to 3.0.6. I'm sure that's perfectly fine, so more as a general comment, when we move things around like this, it's best to move "as is", and then make changes (upgrades, here) separately subsequently (or before, if required). I'm not suggesting that you change it here anymore, just commenting in general, for future refactorings (of build or code, same).

vorburger · 2020-10-13T23:27:41Z

fineract-provider/build.gradle

 apply plugin: "org.nosphere.apache.rat"
 apply plugin: 'rebel'
 apply plugin: 'com.github.hierynomus.license'
 apply plugin: 'war'
 apply plugin: 'org.springframework.boot'
-apply plugin: 'eclipse'


Right, we can ditch this.. makes sense, to me. Again, better separately in the future, but not suggesting to touch here anymore (sorry to keep being a pain about this).

vorburger · 2020-10-13T23:29:23Z

fineract-provider/build.gradle

-eclipse
-{
-    project {
-        buildCommand([ LaunchConfigHandle: "<project>/.externalToolBuilders/OpenJPA Enhance Builder.launch" ],  'org.eclipse.ui.externaltools.ExternalToolBuilder')


so this gets entirely ditch, not just moved.. why? @ptuomola does this LGTY? (I have NOT pulled it myself yet, to actually test.)

... because "fineract-provider/.externalToolBuilders/OpenJPA Enhance Builder.launch" gets picked up automatically by Eclipse when you import via "Existing Gradle Project"... when I open the .project file in fineract-provider then I see this:

So... should be fine.

vorburger · 2020-10-13T23:30:39Z

build.gradle

+            trimTrailingWhitespace()
+        }
+
+        if (plugins.hasPlugin('java')) {


vorburger · 2020-10-13T23:33:52Z

build.gradle

+                }
+            }
+
+            if (project.hasProperty("automatedBuild")) {


This seems to be moving from another section? More out of curiosity, what is this for? /Cc @thesmallstar (FYI we are doing a massive "move", see FINERACT-1188 and FINERACT-1171 for more background)

vidakovic · 2020-10-13T23:34:09Z

@vorburger could you give me the instructions again to solve that issue you mentioned with the rebase? I tried the steps you suggested, but just get weird feedback on the console. E. g. should I do "git rebase develop" in the PR branch? My repos develop and the PR branch are already in sync.

vorburger · 2020-10-13T23:39:51Z

fineract-provider/gradlew

@@ -1 +0,0 @@
-../gradlew


FTR: It took me a sec to understand that you are removing the symlink here.. doesn't work? But probably a good idea anyway, for Windoze consistency.. I noticed that currently on develop we have a fineract-provider/gradlew (symlink), but no fineract-provider/gradlew.bat - that would probably create confusion, so +1 to rm this.

vorburger · 2020-10-13T23:48:13Z

@vorburger could you give me the instructions again to solve that issue you mentioned with the rebase?

git rebase develop 
git push --force vidakovic

I tried the steps you suggested, but just get weird feedback on the console.

do you want to share the git error message / feedback?

E. g. should I do "git rebase develop" in the PR branch?

Yeah.

My repos develop and the PR branch are already in sync.

How do you mean? This PR makes it look like you are working on feature/FINERACT-1188-fix branch... if you are saying that your local develop branch already includes your feature/FINERACT-1188-fix commit, then my recommendation would have to be to avoid that. It's confusing as well. You COULD use git rebase origin/develop instead of git rebase develop, to tell it that you want "the develop branch from the origin remote" (assuming that git remote -v shows you that your origin is git@github.com:apache/fineract.git, but you SHOULD not have to do that.. it's super confusing if you locally make develop divert from origin/develop (other than it sometimes behind "behind, but fast-forwardable", that's normal). So, long story short, if I were you, I would git checkout develop && git reset --hard origin/develop - BUT be careful, you'll LOOSE what you have on your develop and don't already have anywhere else, so be sure all this makes sense! (Just ask if not; happy to elaborate, but tomorrow.)

vorburger

LGTM! I even pulled and "tested" it, and indeed, lo and behold, if I add a space at the end of a line in README.md and run ./gradlew spotlessCheck, it chokes on it... works as advertised (i.e. implements FINERACT-1188 as described). I'll let @ptuomola review and Approve this as well. If any other committers are reading along here, feel free to Approve or Comment as well, of course.

If you can sort out the rebase so this has no merge commit, it would be neat. I'll help you, if required.

vidakovic · 2020-10-13T23:59:39Z

@vorburger I think I get an idea what happened here with the rebase... I first tried to solve this in the browser, but somehow lost my changes... and then "fixed" the conflict by merging the PR branch into my vidakovic/fineract develop branch (which was suggested by Github... don't remember where I read)... and that lead to the current PR branch.

So, I'll try to bring my develop branch again to the same level as upstream - without my additional conflict fix merge. And then your instructions should work. Will let you know here... thanks again.

Update: sweet... that worked... learned again something useful!

ptuomola

LGTM! Thanks for persevering with this.

I can merge this - unless someone else wants to review first?

vidakovic · 2020-10-14T06:08:26Z

Thanks again for the patience ☺️ @ptuomola and @vorburger ... now that we are all happy with it I guess you can merge whenever you have a moment

vidakovic requested review from vorburger and ptuomola October 13, 2020 02:30

vidakovic mentioned this pull request Oct 13, 2020

FINERACT-1188: README and other files are not covered by Spotless #1381

Closed

6 tasks

vidakovic force-pushed the feature/FINERACT-1188-fix branch from ed33901 to a038227 Compare October 13, 2020 02:52

vidakovic force-pushed the feature/FINERACT-1188-fix branch from a038227 to 5a5cf71 Compare October 13, 2020 10:50

vidakovic force-pushed the feature/FINERACT-1188-fix branch from 5a5cf71 to 65e67a3 Compare October 13, 2020 12:36

vidakovic force-pushed the feature/FINERACT-1188-fix branch from 65e67a3 to 0bf3973 Compare October 13, 2020 12:56

vidakovic force-pushed the feature/FINERACT-1188-fix branch from 0bf3973 to 4ac019f Compare October 13, 2020 14:41

ptuomola requested changes Oct 13, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

build.gradle Show resolved Hide resolved

fineract-provider/build.gradle Show resolved Hide resolved

fineract-provider/build.gradle Show resolved Hide resolved

vidakovic force-pushed the feature/FINERACT-1188-fix branch from 4ac019f to e97d509 Compare October 13, 2020 18:33

ptuomola reviewed Oct 13, 2020

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

ptuomola reviewed Oct 13, 2020

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

vidakovic force-pushed the feature/FINERACT-1188-fix branch 2 times, most recently from 9ddd884 to 5864763 Compare October 13, 2020 19:14

vidakovic requested a review from ptuomola October 13, 2020 19:38

vidakovic force-pushed the feature/FINERACT-1188-fix branch 3 times, most recently from 51b1f38 to 54d4b54 Compare October 13, 2020 21:36

vorburger reviewed Oct 13, 2020

View reviewed changes

build.gradle

trimTrailingWhitespace()

}

if (plugins.hasPlugin('java')) {

Copy link

Member

vorburger Oct 13, 2020

Choose a reason for hiding this comment

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

Nice!!

vidakovic reacted with laugh emoji

vorburger reviewed Oct 13, 2020

View reviewed changes

vorburger approved these changes Oct 13, 2020

View reviewed changes

FINERACT-1188: README and other files are not covered by Spotless

5a9e643

vidakovic force-pushed the feature/FINERACT-1188-fix branch from 1a258ad to 5a9e643 Compare October 14, 2020 00:07

ptuomola approved these changes Oct 14, 2020

View reviewed changes

ptuomola merged commit 5520bb3 into apache:develop Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-1188: README and other files are not covered by Spotless #1396

FINERACT-1188: README and other files are not covered by Spotless #1396

vidakovic commented Oct 13, 2020 •

edited by vorburger

Loading

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020 •

edited

Loading

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020 •

edited

Loading

vidakovic commented Oct 13, 2020

vorburger commented Oct 13, 2020

vorburger Oct 13, 2020

vorburger Oct 13, 2020

vidakovic Oct 13, 2020

vorburger Oct 13, 2020

vorburger Oct 13, 2020

vidakovic Oct 13, 2020

vorburger Oct 13, 2020

vorburger Oct 13, 2020

vidakovic commented Oct 13, 2020

vorburger Oct 13, 2020

vorburger commented Oct 13, 2020

vorburger left a comment

vidakovic commented Oct 13, 2020 •

edited

Loading

ptuomola left a comment

vidakovic commented Oct 14, 2020

		@@ -1 +0,0 @@
		../gradlew

FINERACT-1188: README and other files are not covered by Spotless #1396

FINERACT-1188: README and other files are not covered by Spotless #1396

Conversation

vidakovic commented Oct 13, 2020 • edited by vorburger Loading

Description

Checklist

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020 • edited Loading

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

ptuomola commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020

vidakovic commented Oct 13, 2020 • edited Loading

vidakovic commented Oct 13, 2020

vorburger commented Oct 13, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidakovic commented Oct 13, 2020

Choose a reason for hiding this comment

vorburger commented Oct 13, 2020

vorburger left a comment

Choose a reason for hiding this comment

vidakovic commented Oct 13, 2020 • edited Loading

ptuomola left a comment

Choose a reason for hiding this comment

vidakovic commented Oct 14, 2020

vidakovic commented Oct 13, 2020 •

edited by vorburger

Loading

vidakovic commented Oct 13, 2020 •

edited

Loading

vidakovic commented Oct 13, 2020 •

edited

Loading

vidakovic commented Oct 13, 2020 •

edited

Loading