FINERACT-2811: Remove Cargo and Cargo plugin - #6410
Conversation
c2d38e6 to
1b0aedf
Compare
galovics
left a comment
There was a problem hiding this comment.
Checked the whole tree - the Cargo removal is clean, no dangling references anywhere (build.gradle files, workflows, CONTRIBUTING.md, adoc). -PcargoDisabled is gone from every workflow that used it, and the tomcat/driver Gradle configurations Cargo owned are removed without touching fineract-provider's own unrelated driver config. The WAR-in-Tomcat coverage trade-off is real (whole integration suite on a Tomcat-deployed WAR, running only post-merge on develop/release -> one @Smoke scenario, running on every PR) but it's the honest trade given the old coverage was already the only workflow not gated pre-merge, so it wasn't buying much. Good defensive touch on parsing the cucumber summary line to guard against "0 scenarios ran" silently passing.
One thing worth flagging clearly before merge: the tags = [x] -> tags = x fix (needed to make the new smoke job's tag filter actually work - the old Groovy list-coercion turned -Pcucumber.tags="@Foo" into the literal string "[@Foo]", which cucumber can't match against anything) has a side effect much bigger than this PR's stated scope. regression-safety-db-changes.yml passes tags the same way, and appears to have been passing green on every PR without ever actually executing its one gated scenario, because of the exact same bug. After this fix, that workflow starts running - and gating - for real. That's a valuable fix, but it's a significant, unrelated behavior change riding along in a Cargo-removal PR. Could you confirm run-regression-safety-db-changes is green here with a non-zero scenario count, add it a summary-line guard like the one you wrote for the smoke job, and call this out explicitly in the PR description? Whoever bisects a future regression-safety failure to this PR is going to have a bad time otherwise.
Two smaller things:
- The JDWP debug port changes from 5000 to 5005 across several compose/env files - unrelated to Cargo removal, and it'll break every contributor's saved remote-debug run configuration with no changelog entry. Worth splitting out or at least calling out explicitly.
tomcat:10.1-jdk25-temurinis the only floating image tag in the new compose file while everything else in the repo (postgres, localstack, mock-oauth2-server) is pinned to an exact version - worth pinning the patch version too.
Recommendation: COMMENT
Cargo plugin is outdated and unmaintained. Cargo while it is maintained, i believe it would be better to leverage docker + compose or github Actions for testing.
Changes