Cleanup Spring Dependencies #1334
Conversation
Signed-off-by: Siri Varma Vegiraju <[email protected]>
There was a problem hiding this comment.
@siri-varma I've left some comments .. I do recommend not having the springframework.version managed in this way.. it will make it really hard to maintain.
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
siri-varma
force-pushed
the
users/svegiraju/spring-pom-update
branch
from
40660c4 to
508148c
Compare
Signed-off-by: sirivarma <[email protected]>
|
@salaboy addressed all your comments. Thank you |
There was a problem hiding this comment.
@siri-varma the PR looks good, but could you please provide some details why this is needed so I have a better high level understanding.
| @@ -18,19 +18,20 @@ | |||
| <dependency> | |||
| <groupId>io.dapr.spring</groupId> | |||
| <artifactId>dapr-spring-data</artifactId> | |||
| <version>${project.parent.version}</version> | |||
| <optional>true</optional> | |||
| </dependency> | |||
There was a problem hiding this comment.
Why do we need to bring in actors dependency explicitly?
| @@ -71,9 +72,20 @@ | |||
| <dependency> | |||
| <groupId>io.dapr</groupId> | |||
| <artifactId>testcontainers-dapr</artifactId> | |||
| <version>${dapr.sdk.alpha.version}</version> | |||
| <scope>test</scope> | |||
| </dependency> | |||
There was a problem hiding this comment.
Any reason we need to bring these dependencies explicitly?
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-context</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
This dependency should be marked as test
| @@ -19,6 +19,10 @@ | |||
| <groupId>org.springframework.data</groupId> | |||
| <artifactId>spring-data-keyvalue</artifactId> | |||
| </dependency> | |||
There was a problem hiding this comment.
Why do we need this explicit dependency?
| @@ -14,6 +14,17 @@ | |||
| <description>Dapr Spring Messaging</description> | |||
| <packaging>jar</packaging> | |||
|
|
|||
There was a problem hiding this comment.
Why do we need this explicit dependency?
| </dependency> | ||
| </dependencies> | ||
| <!-- Test dependencies --> | ||
| <dependency> |
There was a problem hiding this comment.
Shouldn't this be marked as test?
| @@ -230,6 +230,11 @@ | |||
| <artifactId>spring-boot-testcontainers</artifactId> | |||
| <version>${springboot.version}</version> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Shouldn't this be marked as test?
| <version>${dapr.spring.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.dapr.spring</groupId> |
There was a problem hiding this comment.
Shouldn't this be marked as test?
| <version>${dapr.sdk.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.dapr</groupId> |
There was a problem hiding this comment.
Shouldn't this be marked as test?
| @@ -43,7 +45,6 @@ | |||
| <artifactId>spring-boot-dependencies</artifactId> | |||
| <version>${springboot.version}</version> | |||
| <type>pom</type> | |||
| <scope>import</scope> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Shouldn't this be marked as test?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1334 +/- ##
============================================
+ Coverage 76.91% 77.42% +0.51%
- Complexity 1592 1772 +180
============================================
Files 145 204 +59
Lines 4843 5445 +602
Branches 562 597 +35
============================================
+ Hits 3725 4216 +491
- Misses 821 909 +88
- Partials 297 320 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #940
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: