Import Spring Boot's dependency BOM, fix spring-boot:run at parent project level #276

ches · 2019-10-28T10:48:06Z

Two things here—can extract out the small fix commit to a separate PR if there's any issue with the (hopefully last for awhile) dependency management-related part.

spring-boot:run

We realized that changing core's dependency on ingestion to be inter-module instead of package install-based in #262 left behind a workflow issue. Using something like:

$ mvn -am -pl core spring-boot:run

from the project root directory broke, because it tried to find a main application class in the parent POM project and failed.

The first commit here resolves that, and provides dev docs whether you prefer to run from the IDE or CLI.

spring-boot-dependencies

A next step for <dependencyManagement>. Typically it's recommended that Spring Boot applications inherit the spring-boot-starter-parent POM, such that you take on its Maven plugin configurations, <dependencyManagement> section, and <properties> where dep versions are defined.

For a multi-module project that is probably not appropriate, because not every module is a leaf Spring Boot app. Still, it's a good idea to go with composition in lieu of inheritance: import the <dependencyManagement> part, via the spring-boot-dependencies POM. This way you benefit from the application of transitive dependency versions that they've done the work to determine are compatible with the Spring Boot ecosystem, even if left on your own for the plugin configurations.

That is done here. All dependencies that are declared as bill of materials by sprint-boot-dependencies are removed from being directly declared by us, with exception of a small few that need to be overridden.

A little fix when some subproject modules are Spring Boot apps, but the parent is not. Also document how to overcome IntelliJ CE woes with Spring Boot, and inter-module Maven dependencies.

Spring has done the work of finding direct and transitive versions of dependencies compatible with their ecosystem, and declaring them as a BOM. It's in our best interest to heed it as much as we can.

feast-ci-bot · 2019-10-28T10:48:21Z

Hi @ches. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ches · 2019-10-28T11:14:50Z

pom.xml

+                Override things *before* here if needed, but be wary of that!
+
+                https://www.baeldung.com/spring-boot-dependency-management-custom-parent
+                https://github.com/spring-projects/spring-boot/blob/v2.0.9.RELEASE/spring-boot-project/spring-boot-dependencies/pom.xml


Left the URL here for convenience outside the IDE, but in IntelliJ you can go-to-definition on the artifactId below whenever you need to reference what's included.

davidheryanto · 2019-10-29T03:15:51Z

/ok-to-test

.prow/scripts/test-serving.sh

Per discussion: #271 (comment) The `install` step should not be necessary for the unit tests, but we should use `--batch-mode` on the test step for CI.

ches · 2019-10-29T05:06:40Z

/retest

ches · 2019-10-29T06:17:23Z

Confirmed the issue in the end-to-end test, will look into fixing it shortly.

davidheryanto · 2019-10-29T06:57:24Z

Confirmed the issue in the end-to-end test, will look into fixing it shortly.

Thanks!

The issue seems to be after packaging the application with mvn clean package
running java -jar core/target/feast-core-0.3.0-SNAPSHOT.jar throws this error:

Error: Could not find or load main class feast.core.CoreApplication

It was resulting in a non-functional JAR, appears to be incomplete configuration. It seems unlikely to me that core will be used as a library, if there is a need the shading configuration can be fixed but it probably needs other considerations anyway.

ches · 2019-10-30T05:19:16Z

maven-shade-plugin usage on core appears to have been the issue, I've removed it completely for now because I can't see core being used as a library as-is, but if there is intention for that that I'm missing let me know and I'll try to fix the configuration instead of removing it.

ches · 2019-10-30T05:31:51Z

Hmm, if the test_basic e2e test failed but test_all_types passed, is that a flaky test, not enough sleep time for the first test that executes maybe? Should I retest?

davidheryanto · 2019-10-30T06:31:24Z

Hmm, if the test_basic e2e test failed but test_all_types passed, is that a flaky test, not enough sleep time for the first test that executes maybe? Should I retest?

Yes it seems so, we need to update it so the end-to-end test is more reproducible. I retry re-run the end-to-end test and it passes now.

davidheryanto · 2019-10-30T06:31:42Z

/lgtm
Thanks for the fix and cleanup!

woop · 2019-10-30T07:20:55Z

/approve

feast-ci-bot · 2019-10-30T07:20:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ches, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [woop]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ches · 2019-10-30T07:42:06Z

/pony

ches added 2 commits October 28, 2019 17:14

Fix using spring-boot:run from project root

6a1d993

A little fix when some subproject modules are Spring Boot apps, but the parent is not. Also document how to overcome IntelliJ CE woes with Spring Boot, and inter-module Maven dependencies.

Get Spring managed dep versions via their BOM

f4c7543

Spring has done the work of finding direct and transitive versions of dependencies compatible with their ecosystem, and declaring them as a BOM. It's in our best interest to heed it as much as we can.

ches requested review from davidheryanto, pradithya, thirteen37, tims, woop and zhilingc as code owners October 28, 2019 10:48

feast-ci-bot added the needs-ok-to-test label Oct 28, 2019

feast-ci-bot added the size/L label Oct 28, 2019

ches commented Oct 28, 2019

View reviewed changes

ches mentioned this pull request Oct 28, 2019

Feast 0.3 Continuous Integration (CI) Update #271

Merged

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Oct 29, 2019

davidheryanto reviewed Oct 29, 2019

View reviewed changes

.prow/scripts/test-serving.sh Outdated Show resolved Hide resolved

ci: Try --also-make instead of skipping Enforcer

4ebcf1e

Per discussion: #271 (comment) The `install` step should not be necessary for the unit tests, but we should use `--batch-mode` on the test step for CI.

feast-ci-bot assigned davidheryanto Oct 30, 2019

feast-ci-bot added the lgtm label Oct 30, 2019

feast-ci-bot added the approved label Oct 30, 2019

feast-ci-bot merged commit 18d54b7 into feast-dev:0.3-dev Oct 30, 2019

ches deleted the spring-deps-import branch October 30, 2019 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import Spring Boot's dependency BOM, fix spring-boot:run at parent project level #276

Import Spring Boot's dependency BOM, fix spring-boot:run at parent project level #276

ches commented Oct 28, 2019 •

edited

Loading

feast-ci-bot commented Oct 28, 2019

ches Oct 28, 2019

davidheryanto commented Oct 29, 2019

ches commented Oct 29, 2019

ches commented Oct 29, 2019

davidheryanto commented Oct 29, 2019

ches commented Oct 30, 2019

ches commented Oct 30, 2019

davidheryanto commented Oct 30, 2019

davidheryanto commented Oct 30, 2019

woop commented Oct 30, 2019

feast-ci-bot commented Oct 30, 2019

ches commented Oct 30, 2019

Import Spring Boot's dependency BOM, fix spring-boot:run at parent project level #276

Import Spring Boot's dependency BOM, fix spring-boot:run at parent project level #276

Conversation

ches commented Oct 28, 2019 • edited Loading

spring-boot:run

spring-boot-dependencies

feast-ci-bot commented Oct 28, 2019

ches Oct 28, 2019

Choose a reason for hiding this comment

davidheryanto commented Oct 29, 2019

ches commented Oct 29, 2019

ches commented Oct 29, 2019

davidheryanto commented Oct 29, 2019

ches commented Oct 30, 2019

ches commented Oct 30, 2019

davidheryanto commented Oct 30, 2019

davidheryanto commented Oct 30, 2019

woop commented Oct 30, 2019

feast-ci-bot commented Oct 30, 2019

ches commented Oct 30, 2019

ches commented Oct 28, 2019 •

edited

Loading