-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validating runtime dependencies for conflicts #518
base: main
Are you sure you want to change the base?
Conversation
cd6a2b8
to
5e5325f
Compare
5e5325f
to
f27edbf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnatolyPopov thanks for looking into this.
I like that it's more idiomatic now using gradle APIs to enforce the dependencies; but this also makes it harder to reason about if don't know the gradle internals. Hence I'd ask for some additional comments to clarify how to maintain this approach in the future.
About the "plugins" submodule, I'd say we may be overusing the term in other places (the project itself is sometimes referred as plugin). Could we consider backend
instead as it's also known in the codebase?
What do you think about re-structuring the commits in a way that it's clear to see how enabling this new validation strategy flags conflicting dependencies? similar to https://github.com/Aiven-Open/tiered-storage-for-apache-kafka/pull/442/commits.
I think we are also missing cleaning up the previous validation scripts. Could we include it on this PR as well?
@@ -74,6 +74,11 @@ subprojects { | |||
withJavadocJar() | |||
withSourcesJar() | |||
} | |||
configurations.runtimeClasspath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some comments here to clarify how is affecting the deps validation? e.g. usage of platform feature, etc.
@@ -123,29 +128,32 @@ subprojects { | |||
|
|||
azureSdkVersion = "1.2.21" | |||
|
|||
testcontainersVersion = "1.19.4" | |||
testcontainersVersion = "1.19.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this needed as part of this PR? if it's maybe a separate commit will help.
|
||
testcontainersFakeGcsServerVersion = "0.2.0" | ||
} | ||
|
||
dependencies { | ||
compileOnly "org.apache.kafka:kafka-clients:$kafkaVersion" | ||
compileOnly "org.apache.kafka:kafka-storage-api:$kafkaVersion" | ||
compileOnly("org.apache.kafka:kafka-storage-api:$kafkaVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this is a style change only or it has some implications in the context of this PR?
|
||
implementation enforcedPlatform("com.fasterxml.jackson:jackson-bom:$jacksonVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add some comments on what this "enforcePlatform" does and why we needed it here?
e.g. as it's only added for jacksone, does it mean that we would need to know in advance which dependencies may conflict, so we can flag it as we are doing here?
configurations { | ||
providedRuntime | ||
implementation.extendsFrom(providedRuntime) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe some comments here as well
No description provided.