-
Notifications
You must be signed in to change notification settings - Fork 40
Fix Gradle configurations in data-loader module #2742
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
Conversation
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.
Pull Request Overview
Updates Gradle configuration in the data-loader module to resolve naming conflicts and adopt the newer base.archivesName DSL.
- Adds a project renaming workaround in
settings.gradleto avoid duplicate-name conflicts. - Replaces deprecated
archivesBaseNameassignments with thebase { archivesName }block in both core and CLI submodules. - Adjusts the CLI module’s project dependency path and removes an unused root module reference.
- Removes a redundant
groupdeclaration from the parentdata-loaderbuild.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| settings.gradle | Workaround to rename :data-loader:core project to data-loader-core to avoid naming conflicts. |
| data-loader/core/build.gradle | Switched from archivesBaseName to base.archivesName; removed conditional shadowJar disabling block. |
| data-loader/cli/build.gradle | Switched from archivesBaseName to base.archivesName; updated project dependency to :data-loader:data-loader-core and removed the old :core reference. |
| data-loader/build.gradle | Removed the top‐level group declaration for subprojects. |
Comments suppressed due to low confidence (3)
data-loader/cli/build.gradle:17
- The dependency on project(':core') now seems incorrect after renaming the core module. Consider removing it or updating it to the correct path to avoid pulling in the wrong module.
implementation project(':core')
data-loader/core/build.gradle:49
- [nitpick] The conditional disabling of shadowJar was removed, which may lead to fat jars being published unintentionally. Consider documenting this behavior change or restoring the condition if standard JAR publication is still required.
if (project.gradle.startParameter.taskNames.any { it.endsWith('publish') } ||
data-loader/build.gradle:4
- Removing the
groupsetting will change Maven coordinates for all subprojects, which can break downstream consumers. Verify compatibility or document this as a breaking change.
group = "scalardb.dataloader"
| ext { | ||
| jacksonVersion = '2.17.0' | ||
| } | ||
| group = "scalardb.dataloader" |
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.
Deleted this to avoid overriding the scalardb group name. This is the root cause of the release failure.
| // This is a workaround for an issue where projects with the same name lead to unintended conflict. | ||
| // See https://github.com/gradle/gradle/issues/847 for the details | ||
| findProject(':data-loader:core')?.name = 'data-loader-core' |
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.
Added this to avoid the name conflicting issue.
| if (project.gradle.startParameter.taskNames.any { it.endsWith('publish') } || | ||
| project.gradle.startParameter.taskNames.any { it.endsWith('publishToMavenLocal') }) { | ||
| // not to publish the fat jar to maven central | ||
| shadowJar.enabled = false | ||
| } | ||
|
|
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.
This is unnecessary. I missed it during the review of the previous PR.
komamitsu
left a comment
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.
LGTM, thank you!
1df2fd8 to
bea87ec
Compare
ypeckstadt
left a comment
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.
LGTM. Thank you.
|
Merged and working now. |
Description
This PR fixes the Gradle configuration in the
data-loadermodule to resolve the release failure.Related issues and/or PRs
N/A
Changes made
Added some inline comments. Please take a look for the details.
Checklist
Additional notes (optional)
N/A
Release notes
N/A