Skip to content

Conversation

@thongdk8
Copy link
Contributor

Description

This PR adds Docker support for the ScalarDB Data Loader CLI. Also, make it part of the CI workflows. PTAL. Thank you.

Related issues and/or PRs

N/A

Changes made

  • Added a new Dockerfile for the ScalarDB Data Loader CLI
  • Updated the build.gradle file for the data-loader CLI to support Docker builds
  • Modified GitHub workflow files to include the data-loader CLI in the CI/CD pipeline:
    • Updated release-snapshot.yaml to build and push the data-loader CLI Docker image
    • Updated upload-artifacts.yaml to include the data-loader CLI in the artifact upload process
    • Updated create-release.yaml to build and upload the data-loader CLI JAR file
    • Updated vuln-check.yaml to include the data-loader CLI in vulnerability scanning

Checklist

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes

N/A

Release notes

Added Docker support for the ScalarDB Data Loader CLI, enabling containerized deployment of the data loading functionality.

@thongdk8 thongdk8 self-assigned this Jun 12, 2025
@thongdk8 thongdk8 added the enhancement New feature or request label Jun 12, 2025

This comment was marked as outdated.

@thongdk8 thongdk8 requested a review from Copilot June 12, 2025 03:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds Docker support for the ScalarDB Data Loader CLI and integrates its build and deployment into the CI workflows.

  • Introduces a Dockerfile and new Gradle tasks for building the Docker image
  • Updates CI pipeline workflows (release, artifact upload, vulnerability check, and testing) to incorporate the new Data Loader CLI image and test reports

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
data-loader/cli/build.gradle Adds tasks to lint the Dockerfile, copy build output to a Docker context folder, and build the Docker image
data-loader/cli/Dockerfile Provides a new Dockerfile using an Eclipse Temurin JRE base image and sets up a non-root user
.github/workflows/vuln-check.yaml Updates vulnerability scan to include the Data Loader CLI image
.github/workflows/upload-artifacts.yaml Updates artifact upload to push the Data Loader CLI image
.github/workflows/release-snapshot.yaml Updates snapshot release workflow to push the Data Loader CLI image
.github/workflows/create-release.yaml Adds steps to build and upload the Data Loader CLI JAR file for releases
.github/workflows/ci.yaml Ensures test and spotbugs reports for the Data Loader CLI are generated and uploaded; adds Dockerfile linting for the CLI
Comments suppressed due to low confidence (1)

data-loader/cli/Dockerfile:6

  • The COPY pattern does not explicitly reference the 'cli' suffix that is used in the JAR's name (scalardb-data-loader-cli). Consider updating the pattern to 'scalardb-data-loader-cli*.jar' to avoid any ambiguity and ensure the correct artifact is copied.
COPY scalardb-data-loader-*.jar /app.jar

@thongdk8 thongdk8 marked this pull request as ready for review June 12, 2025 04:47
run: |
mkdir -p /tmp/gradle_test_reports/core
mkdir -p /tmp/gradle_test_reports/schema-loader
mkdir -p /tmp/gradle_test_reports/data-loader/core
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added here and not in separate data loader ci file as it seems easier to manage in this file like this.

- name: Dockerfile Lint for ScalarDB Schema Loader
run: ./gradlew schema-loader:dockerfileLint

- name: Dockerfile Lint for ScalarDB Data Loader CLI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason as the above. If required can still be all moved to a separate file though.

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Copy link
Contributor

@ypeckstadt ypeckstadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@@ -0,0 +1,13 @@
FROM eclipse-temurin:8-jre-jammy

RUN apt-get update && apt-get upgrade -y \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has become a Docker-recommended practice to add this.
cf. https://github.com/scalar-labs/scalardb-cluster/pull/1061

Suggested change
RUN apt-get update && apt-get upgrade -y \
RUN apt-get update && apt-get upgrade -y --no-install-recommends \

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I think we need to apply the same for the schema-loader image building. cc @brfrn169

@thongdk8 thongdk8 requested a review from Torch3333 June 12, 2025 05:29
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@brfrn169
Copy link
Collaborator

@thongdk8 Could you please add the data-loader to remove-untagged-images.yaml as well?

https://github.com/scalar-labs/scalardb/blob/c270cf5ede9436b52265b36acb4e74fa07c0e928/.github/workflows/remove-untagged-images.yaml

@ypeckstadt
Copy link
Contributor

@brfrn169 I have updated the remove untagged workflow.

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@brfrn169
Copy link
Collaborator

@inv-jishnu Can you please check this PR?

Copy link
Contributor

@inv-jishnu inv-jishnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thank you.

@brfrn169 brfrn169 merged commit 85d7b5d into master Jun 16, 2025
55 checks passed
@brfrn169 brfrn169 deleted the data-loader/feat/add-docker-suport branch June 16, 2025 06:05
feeblefakie pushed a commit that referenced this pull request Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants