Conversation
7ce2783 to
fa6de53
Compare
9392fbb to
d8a4cc1
Compare
Improve version management and use of convention plugins for shared build logic.
Add support for Sentry monitoring
aef2cb6 to
a34cbd5
Compare
| plugins { | ||
| id("org.radarbase.radar-kotlin") | ||
| } | ||
|
|
||
| val libs = extensions.getByType<VersionCatalogsExtension>().named("libs") | ||
|
|
||
| // Versions of many dependencies defined by radar-commons-kotlin. | ||
| radarKotlin { | ||
| log4j2Version.set(libs.findVersion("log4j2").get().toString()) | ||
| sentryEnabled.set(true) | ||
| openTelemetryAgentEnabled.set(false) | ||
| } |
There was a problem hiding this comment.
Nice work on this modernization. I think using convention plugins might be a bit over engineered for our case, since we're wrapping one plugin in another plugin (same with publishing-convention.gradle.kts). Would a subprojects {} block in the root build file achieve the same thing (like how it currently works with the combination of subprojects and configure blocks) with less complexity? Not opposed to it, just wondering if you considered that route and chose convention plugins intentionally.
There was a problem hiding this comment.
@this-Aditya point for discussion. I learned from the gradle docs that using subprojects is at the moment considered an anti-pattern (see here). I figured we may just as well do it the 'right way' when needed. We can chose to go back to subprojects section I guess because we do not have a lot of logic there and it is more easy to understand to users not so well versed in Gradle.
I realize I chose to use subprojects {...} to declare the pinned dependencies for visibility reasons also.
|
|
||
| plugins { | ||
| id("com.github.davidmc24.gradle.plugin.avro-base") | ||
| id("java-library") |
There was a problem hiding this comment.
I think it is already applied in root build
| @@ -1,40 +1,42 @@ | |||
| plugins { | |||
| kotlin("plugin.allopen") | |||
| id("java-library") | |||
There was a problem hiding this comment.
Same here, this is already applied in root buildscript
| @@ -1,17 +1,22 @@ | |||
| plugins { | |||
| id("java-library") | |||
There was a problem hiding this comment.
This plugin is already applied in root build
| afterEvaluate { | ||
| configurations.all { | ||
| exclude(group = "org.slf4j", module = "slf4j-log4j12") | ||
| } | ||
| } |
There was a problem hiding this comment.
Are you sure this isn’t being pulled in transitively by any dependency now? I don’t see it excluded anywhere.
There was a problem hiding this comment.
That is strange. I use the 'Vulerable dependencies' tab/tool in IntelliJ IDEA and this no longer shows the vulnerable dependencies:
H maven:com.fasterxml.jackson.core:jackson-core:2.20.2
M maven:com.squareup.okio:okio-jvm:3.2.0
H maven:com.fasterxml.woodstox:woodstox-core:6.2.4
M maven:io.ktor:ktor-client-core-jvm:2.3.4
H maven:com.fasterxml.jackson.core:jackson-core:2.12.7
M maven:io.ktor:ktor-network-tls-jvm:2.3.4
M maven:org.apache.logging.log4j:log4j-core:2.20.0
M maven:io.ktor:ktor-client-core-jvm:2.3.9
L maven:org.glassfish.grizzly:grizzly-http:4.0.2
Co-authored-by: Aditya Mishra <98681758+this-Aditya@users.noreply.github.com>
| radarKotlin { | ||
| javaVersion.set(Versions.java) | ||
| log4j2Version.set(rootProject.libs.versions.log4j2) | ||
| } |
There was a problem hiding this comment.
it's already set in the subprojects {} above, so they inherit that configuration, we can remove this.
There was a problem hiding this comment.
Ok, the difference here is the packaging of the Sentry Jar. Intended the Sentry JAR to be packaged by applications only, not the library projects. Would the current construct achieve this?
There was a problem hiding this comment.
Yeah it does achieve this, but what are your thoughts on keeping the default sentryEnabled to false in the subprojects block? That way, at the outer scope we have false by default for any new subproject we add.
There was a problem hiding this comment.
Yes, that sounds great. I have updated the respective sections. I am unsure whether I need to repeat setting the log4j version. I think not, but I do not know how to test this.
| apply(plugin = "java-library") | ||
| apply(plugin = "org.radarbase.radar-kotlin") | ||
| apply(plugin = "org.radarbase.radar-publishing") | ||
| apply(plugin = "org.radarbase.radar-kotlin") |
There was a problem hiding this comment.
radar-kotlin is already set in the subprojects block above, so it applied here too without specifying here.
There was a problem hiding this comment.
Lets discuss the right approach for adding the Sentry JAR.
There was a problem hiding this comment.
Thanks for the context on Sentry. Right now sentryEnabled.set(true) is set in the subprojects {} block, which means it applies to all 5 modules including the libraries. If the intent is for only applications to have Sentry, maybe we should move sentryEnabled.set(true) and openTelemetryAgentEnabled.set(true) to the application configure block instead? That way the default at the outer scope (subprojects block) stays false, and only the apps get Sentry.
Also, I think we can configure the radarKotlin block in the configure block without re-applying the plugin, it's already applied in subprojects {}
There was a problem hiding this comment.
Update committed.
PR feedback Aditya
17840d2 to
e1a2a11
Compare
PR feedback Aditya
f0085c8 to
662987d
Compare
this-Aditya
left a comment
There was a problem hiding this comment.
Thanks for the changes, LGTM 🎉
This PR will: