Skip to content

FINERACT-730: Fixing bootRun and removing embedded Tomcat - #807

Merged
vorburger merged 1 commit into
apache:developfrom
ptuomola:FINERACT-730
May 5, 2020
Merged

vorburger merged 1 commit into
apache:developfrom
ptuomola:FINERACT-730

Conversation

@ptuomola

@ptuomola ptuomola commented May 3, 2020 •

Copy link
Copy Markdown
Contributor

Description

Fixed bootRun / bootJar by:

  • Rewriting ServerApplication class based on SpringBoot documentation
  • Fixing SSL configuration for Tomcat by removing trust store config
  • Reworking dependencies to remove conflicts (conflicting HikariCP, missing commons-logging and commons-io,

To avoid conflict between embedded Tomcat and Spring Boot Tomcat:

  • Added Cargo plugin for Gradle to start a separate Tomcat instance
  • Changed integrationTest to use separate Tomcat instance through Cargo plugin
  • Removed embedded Tomcat and Gradle Tomcat plugin

TODO:

  • Get the dev profile (using MariaDB4j) to work in the same way with integrationTest etc
  • Update Confluence pages to remove references to tomcatRunWar etc

Checklist

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

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

@awasum

awasum commented May 3, 2020

Copy link
Copy Markdown
Contributor

Seems there are checkstyle exceptions with the incoming changes. See: https://travis-ci.org/github/apache/fineract/builds/682516455

Thanks for this very important PR @ptuomola

@vorburger

Copy link
Copy Markdown
Member

@ptuomola would you consider "squashing" the commits into a single one? Or object if we did, before merging?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ptuomola would you consider raising this change as a separate PR instead of as part of this one? Or we could try to make a real better fix, based on what you described in FINERACT-855...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Of course - will squash the commits and remove this. Based on the emails, I believe Awasum was going to send a fix to FINERACT-855.

@awasum awasum May 3, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ptuomola, I am looking into it but I cannot seem to replicate the issue locally as when running ./gradlew integrationTest --tests ClientSavingsIntegrationTest
Everything passes locally. Could it be that our Travis Vm is not powerful enough?
If you have time to get this done earlier than me. Do it.

My fix for FINERACT-855 involved doing something like this where we are accessing the jobHistoryData...like so:

Assert.assertEquals("Verifying Last Scheduler Job Status", "success", jobHistoryData.get(jobHistoryData.size() == 0 ? jobHistoryData.size() : jobHistoryData.size() - 1).get("status"));

So that we stop getting the ArrayOutOfBounceException.

That may not be a good solution.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My suggestion would be that we wrap up this PR ASAP by strictly and naively 👿 following the process outlined on https://github.com/apache/fineract#pull-requests ... @ptuomola just try to get this PR to fully pass the build by raising separate PRs, and then rebasing, for any tests that fail on you here. The "full" (proper) solution, whether Thread.sleep(15000) or something else, should IMHO be worked on completely separate from this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are right @vorburger . lets just get his green

@ptuomola
ptuomola marked this pull request as draft May 3, 2020 18:41
@ptuomola
ptuomola marked this pull request as ready for review May 3, 2020 19:16
@ptuomola
ptuomola marked this pull request as ready for review May 3, 2020 19:25
@ptuomola
ptuomola marked this pull request as draft May 3, 2020 19:50
@ptuomola
ptuomola marked this pull request as ready for review May 3, 2020 20:14
@vorburger

Copy link
Copy Markdown
Member

@ptuomola this now fails in the (brand new...) ClasspathHellDuplicatesCheckRuleTest introduced in https://github.com/apache/fineract/pull/803/files, see FINERACT-919 for background. Please shout here or there if that is not clear enough and you require assistance - without hearing from you, I'll assume that you'll figure it out, whenever you have time for this PR. - Again, we appreciate this contribution very much!

@ptuomola
ptuomola force-pushed the FINERACT-730 branch 2 times, most recently from 46b9d3b to dc76496 Compare May 4, 2020 09:38
@ptuomola

ptuomola commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

@ptuomola this now fails in the (brand new...) ClasspathHellDuplicatesCheckRuleTest introduced in https://github.com/apache/fineract/pull/803/files, see FINERACT-919 for background. Please shout here or there if that is not clear enough and you require assistance - without hearing from you, I'll assume that you'll figure it out, whenever you have time for this PR. - Again, we appreciate this contribution very much!

Thanks - I've fixed the duplicates, and also had to upgrade the Docker Tomcat image to Tomcat 9 in order to get all of checks to pass. So that should all be OK now.

My only concern about this pull request is: after this, Fineract will need Tomcat 9 or above to run. The WAR does not work on Tomcat 7/8 anymore as it has been compiled against Tomcat 9 JARs, and there are some classes that are not present in 7/8. Is that going to be a problem? I.e. are there people who are using Tomcat 7 and expecting Fineract to work? Do we need to communicate this to everyone somehow so that they are aware?

If this is an issue, it might be possibility to build two versions of the WAR - one for Tomcat (including 7/8) and another one for Spring Boot. But if no one is using Tomcat 7/8 (or if we can expect people to upgrade to 9), then of course it would be good to keep things simple...

@vorburger vorburger self-assigned this May 5, 2020
@vorburger

Copy link
Copy Markdown
Member

Re. Tomcat versions, that's totally fine IMHO. But we should document it - do you want to add just 1 bullet point about it on https://github.com/apache/fineract#requirements as part of this PR, like this:

  • Java >= 1.8 (Oracle JVMs have been tested)
  • Tomcat 9 (since FINERACT-730; Tomcat 7/8, now 9 is required)
  • MySQL 5.5

Comment thread Dockerfile Outdated
Comment thread docker/server.xml Outdated
Comment thread Dockerfile Outdated
…t bootRun to work. Migrated from Gradle Tomcat plugin to Cargo plugin to fix integration test
@vorburger
vorburger merged commit b05162f into apache:develop May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants