Add a dedicated CI workflow for data loader CLI #2761
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ypeckstadt
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a dedicated CI workflow to build the Data Loader CLI and fixes a unit test issue related to directory permissions. Key changes include:
- Adding a new workflow in .github/workflows/data-loader-ci.yaml to build the Data Loader CLI.
- Updating the unit test in DirectoryUtilsTest.java to handle non-writable directory scenarios with a fallback for non-POSIX systems.
- Modifying the Gradle build script to ensure the shadowJar task runs after distZip and distTar.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java | Updated unit test to set directory permissions and skip reliably on systems that do not support POSIX. |
| data-loader/cli/build.gradle | Added a mustRunAfter dependency to enforce task ordering for shadowJar relative to distZip and distTar. |
| .github/workflows/data-loader-ci.yaml | New CI workflow file to build the Data Loader CLI. |
Comments suppressed due to low confidence (2)
data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/DirectoryUtilsTest.java:64
- The fallback using toFile().setWritable(false) may not reliably enforce non-writability on all platforms, potentially leading to skipped tests. Consider documenting the known limitations for non-POSIX systems or exploring a more robust approach.
Assumptions.assumeFalse(isWritable, "Directory is still writable; skipping test.");
data-loader/cli/build.gradle:37
- Ensure that the tasks 'distZip' and 'distTar' are consistently defined in the build configuration, as the ordering dependency here relies on their existence and proper configuration.
mustRunAfter distZip, distTar
ypeckstadt
reviewed
Jun 16, 2025
|
|
||
| // Build a fat jar | ||
| shadowJar { | ||
| mustRunAfter distZip, distTar |
Contributor
There was a problem hiding this comment.
Is added via other PR so can be ignored here.
komamitsu
reviewed
Jun 16, 2025
| - name: Cleanup Gradle cache | ||
| run: | | ||
| rm -f ~/.gradle/caches/modules-2/modules-2.lock | ||
| rm -f ~/.gradle/caches/modules-2/gc.properties No newline at end of file |
| steps: | ||
| - uses: actions/checkout@v2 | ||
|
|
||
| - name: Set version |
Contributor
There was a problem hiding this comment.
I might be missing something, but why is this needed?
Also, there are some differences between the original ci.yaml's check job and this new workflow. Can you update this workflow based on the original ci.yaml? I guess it might fix the weird write permission issue.
Torch3333
reviewed
Jun 16, 2025
Comment on lines
26
to
27
| 11 | ||
| 8 |
Contributor
There was a problem hiding this comment.
Why is it necessary to set up both Java 8 and 11 ?
Comment on lines
29
to
37
| - name: Cache Gradle packages | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.gradle/caches | ||
| ~/.gradle/wrapper | ||
| key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-gradle- |
Contributor
There was a problem hiding this comment.
I may miss something, but if you don't require a custom caching logic, using the setup-gradle action is usually preferred for a grade build compared to using the action/cache action.
Suggested change
| - name: Cache Gradle packages | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| ~/.gradle/caches | |
| ~/.gradle/wrapper | |
| key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} | |
| restore-keys: | | |
| ${{ runner.os }}-gradle- | |
| - name: Setup Gradle | |
| uses: gradle/actions/setup-gradle@v4 |
Comment on lines
42
to
45
| - name: Cleanup Gradle cache | ||
| run: | | ||
| rm -f ~/.gradle/caches/modules-2/modules-2.lock | ||
| rm -f ~/.gradle/caches/modules-2/gc.properties No newline at end of file |
Contributor
There was a problem hiding this comment.
If you follow my suggestion of using gradle/actions/setup-gradle@v4, this is not necessary.
Suggested change
| - name: Cleanup Gradle cache | |
| run: | | |
| rm -f ~/.gradle/caches/modules-2/modules-2.lock | |
| rm -f ~/.gradle/caches/modules-2/gc.properties |
Co-authored-by: Mitsunori Komatsu <[email protected]>
komamitsu
reviewed
Jun 17, 2025
| Path readOnlyDirectory = Files.createDirectory(Paths.get(tempDir.toString(), "readOnlyDir")); | ||
| readOnlyDirectory.toFile().setWritable(false); | ||
|
|
||
| // Try to make it read-only using POSIX if supported |
Contributor
There was a problem hiding this comment.
Seems like the cause of the test failure is still unclear, so I think we shouldn't merge this kind of ad-hoc workaround.
I ran ./gradlew :data-loader:cli:build --no-daemon --stacktrace without this workaround with the latest data-loader-ci.yaml, but it didn't fail
https://github.com/scalar-labs/scalardb/actions/runs/15702570827/job/44240661813. I guess the updated workflow file may have resolved the issue. Can you try without this workaround again just in case?
Even if it still fails, the unit tests should be already executed in ci.yaml's check job, so I think we can skip the unit tests in data-loader-ci.yaml by using ./gradlew -x test like ./gradlew -x test :data-loader:cli:build.
Contributor
Author
There was a problem hiding this comment.
@komamitsu san (cc @ypeckstadt ),
Sorry I missed this. You are right about the tests already executing in check job. And as I mentioned, since the test failure only occurs on my local environment with act, it might be some issue with my environment and we can ignore that.
Thank you for pointing this out.
Contributor
Author
There was a problem hiding this comment.
The change above was reverted by @ypeckstadt san in this commit 6def419.
Contributor
|
@inv-jishnu I have restored the changes to the test file and please test again if you can run the action without any problems without any additional fixes this time. If so, we can just treat this PR as the title suggests. |
Contributor
Author
I have tested with act in local (there was issues in using v4 with act, so I had to change it to v3) and the CI failed with reverted test changes with the same issue as before. When I updated the test, with the fix and re ran it, the CI workflow was completed successfully. So we need to add the fix for unit test. |
Contributor
@inv-jishnu It looks like the CI finished successfully |
Contributor
Author
@komamitsu san, I tested the workflow in my local environment with act. There may be some issue with act or with some configuration set in my local environment. The issue I faced can be ignored as it is working as expected in the actual workflow run. I have added a snippet from log for reference. |
feeblefakie
pushed a commit
that referenced
this pull request
Jun 18, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a dedicated CI workflow just for the data loader CLI. The main CI.yaml file only runs
checkand for the data loader CLI build errors made it's way into the master branch. To avoid this, we like to run the build command for the data loader in the CI as well.After adding and testing this workflow, it was also apparent that one of the unit tests was failing when running in certain environments. (only failing when running in the workflows) A test for a read only folder that failed with it's directory permissions. This PR also fixes this unit test to make sure the build is ok.
Based on PR comment.
Related issues and/or PRs
Please review and merge this PR after the following PR is merged.
Changes made
I have tested the CI with act in my local machine and confirmed the CI is working.
Checklist
Additional notes (optional)
Release notes
Add a CI workflow for data loader CLI build