You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/10/09 20:36:28 UTC
[GitHub] [fineract] vidakovic opened a new pull request #1381: FINERACT-1181: README and other files are not covered by Spotless
vidakovic opened a new pull request #1381:
URL: https://github.com/apache/fineract/pull/1381
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] Create/update unit or integration tests for verifying the changes made.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] 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
- [x] 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.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1181: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706443877
@vorburger I think I need a bit of help here... don't really understand why the actuator check fails
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706778921
@vorburger @ptuomola So, I think that would be it... the only thing: I had some difficulties rebasing that's why you see multiple commits. When I pulled in the latest changes from upstream I had to solve one conflict. During rebase I ran then multiple times into various conflict situations and didn't really entirely understand why. But the Spotless configuration is now moved to the root build.gradle and will work for any module that is added (not only for fineract-provider). I've removed also everything that's unnecessary in fineract-provider (mostly Spotless related). @vorburger as you can see the changes are inline and not externalized with "apply from" etc.
I just don't know why the CI build is failing again. Don't really see a reason why that health check would fail. Any ideas?
Update: in the meanwhile I tried to run the Docker image locally and it fails there with a database connection refused error
```
fineract-server_1 | Caused by: org.drizzle.jdbc.internal.common.QueryException: Could not connect: Connection refused (Connection refused)
fineract-server_1 | at org.drizzle.jdbc.internal.mysql.MySQLProtocol.<init>(MySQLProtocol.java:147) ~[drizzle-jdbc-1.4.jar:na]
fineract-server_1 | at org.drizzle.jdbc.DrizzleDriver.connect(DrizzleDriver.java:91) ~[drizzle-jdbc-1.4.jar:na]
fineract-server_1 | ... 188 common frames omitted
fineract-server_1 | Caused by: java.net.ConnectException: Connection refused (Connection refused)
fineract-server_1 | at java.base/java.net.PlainSocketImpl.socketConnect(Native Method) ~[na:na]
fineract-server_1 | at java.base/java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:399) ~[na:na]
fineract-server_1 | at java.base/java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:242) ~[na:na]
fineract-server_1 | at java.base/java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:224) ~[na:na]
fineract-server_1 | at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:403) ~[na:na]
fineract-server_1 | at java.base/java.net.Socket.connect(Socket.java:609) ~[na:na]
fineract-server_1 | at java.base/java.net.Socket.connect(Socket.java:558) ~[na:na]
fineract-server_1 | at org.drizzle.jdbc.internal.mysql.MySQLProtocol.<init>(MySQLProtocol.java:142) ~[drizzle-jdbc-1.4.jar:na]
fineract-server_1 | ... 189 common frames omitted
fineract-server_1 |
```
FYI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic closed pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic closed pull request #1381:
URL: https://github.com/apache/fineract/pull/1381
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706778921
@vorburger @ptuomola So, I think that would be it... the only thing: I had some difficulties rebasing that's why you see multiple commits. When I pulled in the latest changes from upstream I had to solve one conflict. During rebase I ran then multiple times into various conflict situations and didn't really entirely understand why. But the Spotless configuration is now moved to the root build.gradle and will work for any module that is added (not only for fineract-provider). I've removed also everything that's unnecessary in fineract-provider (mostly Spotless related). @vorburger as you can see the changes are inline and not externalized with "apply from" etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706539080
@vorburger to answer your questions:
1. Just let me know how you and @ptuomola decide... and I'll change it accordingly. I can see your point here with the non-recurring includes; in general I find those modular gradle files useful.
2. Currently the root wrapper is disabled with the content in settings.gradle. You can't have 2 wrappers (actually project roots), and as I understood you the goal with that first PR was not to change anything for existing setups... because if we change this - and I would really like to do that - then everyone needs to re-import their Gradle project in their IDEs. If that is OK with everyone I'd be happy to do it.
3. See my answer in 2.
4. There's no particular reason for -all, just a habit of mine, because with that you get also Gradle's sources for your IDE. Don't need it that often, but sometimes for debugging when something is messed up with the Gradle build. Overall you are right, the community probably won't need it.
5. Sure, I can move just the wrapper.
6. Done. Approved and merged. Will rebase this branch.
7. See my answer in 2. I just wanted to start moving things to the root folder while still maintaining the same setup for everyone else. I just thought you would find it too invasive. So again, if everyone is OK re-importing their project from root then I'll happily do that change.
Concerning squashing: no problem, will do.
Concerning Maven Central: trivial, will appear with the next rounds of cleanups.
Concerning Gradle Plugin Portal: official repo for all Gradle plugins; can't really say that some might not be available in Maven Central; but could be. Until we really have resolution errors we can remove.
@vorburger Could you just confirm that those breaking changes are OK before I edit? And please let me know if you want to see the wrapper move to root in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706555171
@vidakovic thanks for letting me follow along the changes slowly and step by step! Sorry if this is a pain, and I'm learning more about what makes most sense in which order based on review feedback. So, to me personally, forcing people to re-import into an IDE is actually a non-issue, as long as we do everything in steps that everyone can easily understand. IMHO expecting re-import is totally fine; we can always send a line about it to the dev list if required, but I don't even think it really is.. you git pull, it's red, you git log, you figure it out. We can let others chime in on PR review dedicated to JUST that small change for that as well. So what would be the smallest possible incremental next step here then? Do you want to raise a separate new PR with JUST git mv the wrapper out of the fineract-provider into root, and whatever other minimal adjustments that requires, but only what's strictly required? (E.g. don't change bin to dev, yet; could be separate after.) I'm guessi
ng that move, alone, wouldn't give us Spotless coverage for root files just yet, which is fine, we can get back to that in this PR, after.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-707447148
This failed PR is replaced by #1396
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a change in pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on a change in pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#discussion_r503077935
##########
File path: gradlew
##########
@@ -130,6 +130,8 @@ fi
if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then
APP_HOME=`cygpath --path --mixed "$APP_HOME"`
CLASSPATH=`cygpath --path --mixed "$CLASSPATH"`
+
+ JAVACMD=`cygpath --unix "$JAVACMD"`
Review comment:
Interesting... didn't see that one. I'll have a look at it. Thanks again @ptuomola
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1181: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706435268
The failed build is not related to the changes in this PR. Looks more like the tests were running out of memory or something along those lines. Trying again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706557099
@vorburger No problem, learning too 😉. I will create a separate PR for moving the wrapper... as a subtask of FUNERACT-1171. Once that is accepted I will rebase FINERACT-1188 and clean it up. And then we continue from there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706539080
@vorburger to answer your questions:
1. Just let me know how you and @ptuomola decide... and I'll change it accordingly. I can see your point here with the non-recurring includes; in general I find those modular gradle files useful.
2. Currently the root wrapper is disabled with the content in settings.gradle. You can't have 2 wrappers (actually project roots), and as I understood you the goal with that first PR was not to change anything for existing setups... because if we change this - and I would really like to do that - then everyone needs to re-import their Gradle project in their IDEs. If that is OK with everyone I'd be happy to do it.
3. See my answer in 2.
4. There's no particular reason for -all, just a habit of mine, because with that you get also Gradle's sources for your IDE. Don't need it that often, but sometimes for debugging when something is messed up with the Gradle build. Overall you are right, the community probably won't need it.
5. Sure, I can move just the wrapper.
6. Done. Approved and merged. Will rebase this branch.
7. See my answer in 2. I just wanted to start moving things to the root folder while still maintaining the same setup for everyone else. I just thought you would find it too invasive otherwise. So again, if everyone is OK re-importing their project from root then I'll happily do that change.
Concerning squashing: no problem, will do.
Concerning Maven Central: trivial, will appear with the next rounds of cleanups.
Concerning Gradle Plugin Portal: official repo for all Gradle plugins; can't really say that some might not be available in Maven Central; but could be. Until we really have resolution errors we can remove.
@vorburger Could you just confirm that those breaking changes are OK before I edit? And please let me know if you want to see the wrapper move to root in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706778921
@vorburger @ptuomola So, I think that would be it... the only thing: I had some difficulties rebasing that's why you see multiple commits. When I pulled in the latest changes from upstream I had to solve one conflict. During rebase I ran then multiple times into various conflict situations and didn't really entirely understand why. But the Spotless configuration is now moved to the root build.gradle and will work for any module that is added (not only for fineract-provider). I've removed also everything that's unnecessary in fineract-provider (mostly Spotless related). @vorburger as you can see the changes are inline and not externalized with "apply from" etc.
I just don't know why the CI build is failing again. Don't really see a reason why that health check would fail. Any ideas?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#discussion_r503027876
##########
File path: gradlew
##########
@@ -130,6 +130,8 @@ fi
if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then
APP_HOME=`cygpath --path --mixed "$APP_HOME"`
CLASSPATH=`cygpath --path --mixed "$CLASSPATH"`
+
+ JAVACMD=`cygpath --unix "$JAVACMD"`
Review comment:
Something funny going on here... I'm assuming this is a mistake from resolving the merge conflicts? Same with the gradle-wrapper.jar changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1381: FINERACT-1181: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#issuecomment-706456106
Alright, figured it out... it's a bit tricky to have 2 Gradle setups.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#discussion_r502767019
##########
File path: gradle/repositories.gradle
##########
@@ -0,0 +1,23 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+repositories {
+ jcenter()
+ mavenCentral()
+ gradlePluginPortal()
Review comment:
@vidakovic what is this `gradlePluginPortal()` required for? We don't currently seem to use it in our `fineract-provider/build.gradle`. If you need it later, it's probably clearer add it only when it's required, not yet here? But even so.. my Gradle foo is quite limited, but if anything shouldn't this only be in `buildscript { repositories` and never root `repositories`, because Gradle Plugins should never be "runtime" dependencies? Is `gradlePluginPortal()` the equivalent of ``maven { url "https://plugins.gradle.org/m2/" }`? @ptuomola
##########
File path: gradle/repositories.gradle
##########
@@ -0,0 +1,23 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+repositories {
+ jcenter()
+ mavenCentral()
Review comment:
@vidakovic if I'm not mistaken, we don't actually need `mavenCentral()`... I'm trying that out in #1383 (I've always meant to clean that up, but never did).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a change in pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
vidakovic commented on a change in pull request #1381:
URL: https://github.com/apache/fineract/pull/1381#discussion_r503197048
##########
File path: gradlew
##########
@@ -130,6 +130,8 @@ fi
if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then
APP_HOME=`cygpath --path --mixed "$APP_HOME"`
CLASSPATH=`cygpath --path --mixed "$CLASSPATH"`
+
+ JAVACMD=`cygpath --unix "$JAVACMD"`
Review comment:
@ptuomola this seems to be still failing... how about kicking this one in the trash can and starting from scratch? I think that would save us a ton of time... going to do that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1381: FINERACT-1188: README and other files are not covered by Spotless
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1381:
URL: https://github.com/apache/fineract/pull/1381
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org