feat(dockerfiles/ci/tici): update tici Dockerfile for installing yq and oras tools#905
Conversation
Signed-off-by: lyb <yebin.li@pingcap.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
|
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the tici Dockerfile to install two additional CLI tools: yq and oras. The approach adds arguments for version control and uses conditional architecture selection to download the appropriate binaries, followed by installation and cleanup steps. The overall quality is reasonable, but there are some critical issues around environment variables, error handling, and cleanup that could impact maintainability and robustness.
Critical Issues
-
Undefined
$OSvariable (dockerfiles/ci/tici/Dockerfile lines 18-27)- The curl commands reference
${OS}but this variable is never defined in the Dockerfile, which will cause the downloads to fail. - Fix: Define the
OSvariable similarly toARCH, e.g.:orARG OS=linuxEnsureRUN OS=$(uname | tr '[:upper:]' '[:lower:]')
$OSmatches the naming convention used by the yq and oras binaries (usually "linux" or "darwin").
- The curl commands reference
-
Potential failure without
set -eor error checking (dockerfiles/ci/tici/Dockerfile lines 18-27)- If any curl or tar command fails, the Docker build may continue without immediate failure, resulting in incomplete or broken installations.
- Fix: Add
set -euxo pipefailat the start of the RUN command to ensure failures cause the build to stop:RUN set -euxo pipefail; \ ARCH=...; \ OS=...; \ ... - Alternatively, split the RUN commands to catch errors earlier.
Code Improvements
-
Version argument defaulting and usage (dockerfiles/ci/tici/Dockerfile lines 16-17, 19-22)
- The PR uses ARGs for versions but does not document or set defaults explicitly other than in the Dockerfile.
- Consider adding comments explaining version variables or providing a fallback default in the build command.
-
Cleanup of intermediate files (dockerfiles/ci/tici/Dockerfile line 26)
- The
rm -rfcommand removes downloaded tarballs and extracted directories, which is good. - Consider using a single temporary directory (e.g.,
/tmp/oras-install) to avoid potential conflicts or clutter and to better isolate installation artifacts.
- The
-
Use
--locationinstead of-LOfor curl consistency- The first curl uses
-fsSL, the second uses-LO. Consider using consistent flags for readability and error handling:curl -fsSL -o ...
- The first curl uses
Best Practices
-
Add comments describing the steps (dockerfiles/ci/tici/Dockerfile lines 18-27)
- Add comments describing what each step is doing, e.g.:
# Download and install yq binary # Download, extract, and install oras tool
- Add comments describing what each step is doing, e.g.:
-
Testing coverage
- No tests or validation are included to verify installation success or tool versions.
- Consider adding a
RUN yq --version && oras versionstep after installation to verify tools are installed correctly.
-
Naming and style
- The comment line
# yq、oras tooluses a non-standard comma character (、), replace it with a comma or "and" for consistency:# yq and oras tools
- The comment line
Summary of suggested Dockerfile snippet improvements:
ARG YQ_VERSION=v4.52.2
ARG ORAS_VERSION=v1.3.0
ARG OS=linux
RUN set -euxo pipefail; \
ARCH=$([ "$(arch)" = "x86_64" ] && echo amd64 || echo arm64); \
# Download and install yq
curl -fsSL "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_${OS}_${ARCH}" -o /usr/local/bin/yq; \
chmod +x /usr/local/bin/yq; \
# Download, extract, and install oras
curl -fsSL "https://github.com/oras-project/oras/releases/download/${ORAS_VERSION}/oras_${ORAS_VERSION#?}_${OS}_${ARCH}.tar.gz" -o /tmp/oras.tar.gz; \
mkdir -p /tmp/oras-install; \
tar -zxf /tmp/oras.tar.gz -C /tmp/oras-install; \
install /tmp/oras-install/oras /usr/local/bin/; \
rm -rf /tmp/oras.tar.gz /tmp/oras-install; \
# Verify installations
yq --version; \
oras versionThis ensures the build fails on errors, uses defined OS variable, isolates temp files, and verifies the installation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds yq and oras tools to the tici Docker image. My review focuses on improving the installation method. I've suggested replacing the complex RUN command with Docker's multi-stage build feature (COPY --from), which is a cleaner, more maintainable, and idiomatic approach for including binaries from other Docker images. This also aligns with practices seen elsewhere in the repository. A minor suggestion to use English in a code comment is also included.
lybcodes
changed the title
Signed-off-by: lyb <yebin.li@pingcap.com>
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR updates the dockerfiles/ci/tici/Dockerfile to install the yq and oras CLI tools by leveraging multi-stage builds from their official images. It introduces build arguments for versioning and copies the binaries from the respective images into the final image. The approach is straightforward and avoids manual installation steps, improving maintainability. Overall, the changes are clean and integrate well into the existing Dockerfile.
Code Improvements
-
File:
dockerfiles/ci/tici/Dockerfilelines 1-11
Issue: The Dockerfile uses multipleFROMinstructions to importyqandorasimages without naming the final stage explicitly. This can be confusing and may cause unintended behavior, as the final stage is implicitly the lastFROM(which is$BASE_IMGhere). However, the order ofFROMstatements is non-standard:yqandorasare declared beforeFROM $BASE_IMG, thenFROM $BASE_IMGrestarts a new stage. This works but is less readable.Suggestion: Use named stages explicitly for clarity, and place the
FROM $BASE_IMGas the first stage or last stage, but clearly name all stages. For example:ARG BASE_IMG=ghcr.io/pingcap-qe/cd/builders/tiflash:v2024.10.8-135-g43bfc3c-centos7 ARG YQ_VERSION=4.52.2 ARG ORAS_VERSION=v1.3.0 FROM mikefarah/yq:${YQ_VERSION} AS yq FROM ghcr.io/oras-project/oras:${ORAS_VERSION} AS oras FROM $BASE_IMG AS base # ... existing yum install and other RUN commands ... # Copy yq and oras from respective stages COPY --from=yq /usr/bin/yq /usr/local/bin/yq COPY --from=oras /bin/oras /usr/local/bin/oras
This makes it clear what each stage is responsible for, improves maintainability, and prevents potential confusion.
Best Practices
-
File:
dockerfiles/ci/tici/Dockerfilelines 1-7
Issue: The version argsYQ_VERSIONandORAS_VERSIONare defined but not documented or defaulted clearly in the Dockerfile or PR description, which may cause confusion about how to override them.Suggestion: Add default versions in the Dockerfile (which you partially do), but also document how to override them when building the Dockerfile, e.g., in a comment or README snippet:
ARG YQ_VERSION=4.52.2 # Can be overridden at build time with --build-arg YQ_VERSION=... ARG ORAS_VERSION=v1.3.0 # Can be overridden similarly
Also, consider adding a short comment above the multi-stage build explaining the purpose of these stages.
-
File:
dockerfiles/ci/tici/Dockerfilelines 19-22
Issue: The PR adds the binaries but does not verify their presence or versions after copying. This can lead to subtle issues if the source images change or the binaries are missing.Suggestion: Add a simple verification step after copying, for example:
RUN yq --version && oras versionThis ensures the tools are installed correctly and helps catch errors early during image build.
Minor Suggestions
-
Consider pinning the exact digest of the base images (
mikefarah/yqandoras) for reproducible builds if security or stability is a concern, especially in CI. -
The PR description is empty; adding a short summary there would help future reviewers understand the motivation and usage. For example:
"This PR adds the latest yq and oras CLI tools to the tici CI Docker image via multi-stage builds to enable YAML processing and OCI artifact management in CI pipelines."
Overall, the PR achieves its goal with minimal changes and can be improved mainly by clarifying Dockerfile stages and adding validation steps.
Signed-off-by: lyb <yebin.li@pingcap.com>
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the dockerfiles/ci/tici/Dockerfile to install the yq and oras CLI tools by building them in separate stages and copying their binaries into the final image. It uses multi-stage builds with versioned ARGs, which improves maintainability and ensures version control. Overall, the approach is clean and leverages Docker best practices for tooling installation, though a few improvements on layering and version handling can be made.
Code Improvements
-
Reducing Image Layers and Cleanup:
- File:
dockerfiles/ci/tici/Dockerfilelines 6-19 - Issue: The current Dockerfile adds two separate stages for
yqandorasand copies their binaries into the base image, which is good. However, the base image layer (RUN yum install ...) follows these copies, which means the installed tools are not cached efficiently if dependencies change. Also, the final image may include unnecessary leftover files or metadata from the base image. - Suggestion: Consider rearranging the Dockerfile so that the base image and its dependencies are installed first, then the binaries are copied in a later step. This improves layer caching and image size efficiency. For example:
This way, the base image dependencies are installed once before copying the tools, reducing rebuild times.
ARG BASE_IMG=... ARG YQ_VERSION=4.52.2 ARG ORAS_VERSION=v1.3.0 FROM $BASE_IMG RUN yum install -y wget lsof openssh-server openssh-clients sudo mysql protobuf-compiler perl-core openssl-devel FROM mikefarah/yq:${YQ_VERSION} AS yq FROM ghcr.io/oras-project/oras:${ORAS_VERSION} AS oras FROM $BASE_IMG COPY --from=yq /usr/bin/yq /usr/local/bin/yq COPY --from=oras /bin/oras /usr/local/bin/oras
- File:
-
Binary Path Consistency:
- File:
dockerfiles/ci/tici/Dockerfilelines 17-18 - Issue: The
yqbinary is copied from/usr/bin/yqbut theorasbinary is copied from/bin/oras. This can be brittle if upstream images change their binary locations. - Suggestion: Verify and document the expected binary paths or add a check in the Dockerfile or CI to validate the presence of these binaries, or explicitly install from releases or static binaries to control paths.
- File:
-
Pinning Base Image Version:
- File:
dockerfiles/ci/tici/Dockerfileline 1 - Issue: The base image tag includes a specific version (
v2024.10.8-135-g43bfc3c-centos7), which is good for reproducibility. However, the ARG is not used in the multi-stage builds after the first FROM, which might cause inconsistencies if the base image is updated but yq/oras stages are not rebuilt. - Suggestion: Consider using the base image ARG consistently in all FROM statements or clarify its usage in comments.
- File:
Best Practices
-
Documentation and Comments:
- File:
dockerfiles/ci/tici/Dockerfilelines 3-7 - Issue: While there are comments about renovate datasource for automated updates, there is no explanation about why
yqandorasare added or how they are used in the CI. - Suggestion: Add a brief comment explaining the purpose of these tools in the CI environment to aid future maintainers:
# Install yq for YAML processing and oras for OCI artifact management in CI
- File:
-
Testing Coverage:
- General:
- Issue: The PR does not mention any updates to tests or CI validation to ensure that
yqandorasare correctly installed and executable in the resulting image. - Suggestion: Add a simple test step in the CI pipeline or Dockerfile to verify installed versions, e.g.,
This helps catch installation or path issues early.
RUN yq --version && oras version
-
Style and Formatting:
- File:
dockerfiles/ci/tici/Dockerfileline 2 - Issue: There is an unnecessary blank line after the ARG declaration at the top, minor but can be removed for tidiness.
- Suggestion: Remove extra blank lines to keep the Dockerfile concise.
- File:
Overall, the PR introduces useful tooling in a clean multi-stage build manner, but can be improved by adjusting the order of commands for better caching, adding validation steps, and clarifying the intent with comments.
Signed-off-by: lyb <yebin.li@pingcap.com>
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR updates the dockerfiles/ci/tici/Dockerfile to include the installation of two additional CLI tools, yq (version 4.52.2) and oras (version 1.3.0), by copying their binaries from official container images. The approach uses multi-stage Docker build syntax (COPY --from=) to directly copy the binaries into the build image, avoiding manual installation steps or downloading binaries explicitly. Overall, the change is straightforward and cleanly integrates the new tools with minimal disruption to the existing Dockerfile.
Code Improvements
-
Verify binary paths from source images
- File:
dockerfiles/ci/tici/Dockerfilelines 24-27 - The
COPY --from=commands assume the binaries reside at/usr/bin/yqand/bin/orasrespectively inside the source images. It’s best practice to verify these exact paths to avoid build failures. For instance, themikefarah/yqimage typically placesyqat/usr/bin/yq, butorasbinary location might differ depending on the image layout. - Suggestion: Confirm the exact binary paths in the source images by inspecting or running them. Alternatively, add a comment with the command used to verify the paths for maintainability. For example:
# Verified oras binary path by running: docker run --rm -it ghcr.io/oras-project/oras:v1.3.0 which oras COPY --from=ghcr.io/oras-project/oras:v1.3.0 /bin/oras /usr/local/bin/oras
- File:
-
Make tools executable explicitly
- File:
dockerfiles/ci/tici/Dockerfilelines 24-27 - The copied binaries might not retain executable permissions, depending on the source image's file metadata and Docker's handling. This can cause permission issues on usage.
- Suggestion: Add a
RUN chmod +x /usr/local/bin/yq /usr/local/bin/orasafter copying to ensure executability:COPY --from=mikefarah/yq:4.52.2 /usr/bin/yq /usr/local/bin/yq COPY --from=ghcr.io/oras-project/oras:v1.3.0 /bin/oras /usr/local/bin/oras RUN chmod +x /usr/local/bin/yq /usr/local/bin/oras
- File:
-
Consider pinning base image versions explicitly
- File:
dockerfiles/ci/tici/Dockerfileline 1 - The
ARG BASE_IMGis using a specific tag with git commit hash and version, which is good for reproducibility. Just ensure this practice is consistent and documented.
- File:
Best Practices
-
Add comments explaining why these tools are needed
- File:
dockerfiles/ci/tici/Dockerfilelines 23-28 - While the new tools
yqandorasare added, there is no explanation in the Dockerfile about their intended use cases or why these specific versions are chosen. - Suggestion: Add a brief comment describing the purpose of these tools in the CI environment and mention the decision to vendor binaries via multi-stage copy for efficiency. For example:
# Install yq (YAML processor) and oras (OCI registry tool) for CI tasks. # Binaries copied from official images to avoid manual installation.
- File:
-
Testing coverage and validation
- The PR description shows screenshots of local test builds, which is good. Consider adding automated tests or Docker build validation steps in CI to catch any future regressions related to these tools.
Minor
- Whitespace cleanup
- The added blank line after
ARG BASE_IMG=...is harmless but inconsistent with previous style. - Consider removing the extra blank line after the
ARGfor consistent formatting.
- The added blank line after
Summary of suggested Dockerfile snippet improvements:
ARG BASE_IMG=ghcr.io/pingcap-qe/cd/builders/tiflash:v2024.10.8-135-g43bfc3c-centos7
FROM $BASE_IMG
# Install yq (YAML processor) and oras (OCI registry tool) for CI tasks.
# Binaries copied from official images to avoid manual installation.
# renovate: datasource=docker depName=mikefarah/yq
COPY --from=mikefarah/yq:4.52.2 /usr/bin/yq /usr/local/bin/yq
# renovate: datasource=github-tags depName=oras-project/oras
COPY --from=ghcr.io/oras-project/oras:v1.3.0 /bin/oras /usr/local/bin/oras
RUN chmod +x /usr/local/bin/yq /usr/local/bin/orasOverall, the PR introduces a useful and clean enhancement. Addressing the above points will ensure robustness and maintainability of the Dockerfile changes.
Signed-off-by: lyb <yebin.li@pingcap.com>
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the dockerfiles/ci/tici/Dockerfile by adding installation of yq and oras CLI tools via multi-stage Docker build copy from their official container images. The approach uses the COPY --from= directive to directly copy binaries, avoiding re-downloading or rebuilding from source, which is efficient and clean. Overall, the changes are minimal, focused, and follow a good pattern for including external tools in the image.
Code Improvements
-
Pin exact binary paths and verify existence:
The Dockerfile copies/usr/bin/yqand/bin/orasfrom the source images. It would be safer to verify that these paths are stable and won't change in future versions. Sometimes, upstream images might relocate binaries or use alternative paths (e.g.,/usr/local/bin/oras). Consider adding a check or comment about this assumption to avoid future breakage. -
Add executable permission assurance:
Although unlikely, copied binaries may lose executable permission or have restrictive permissions. It's a best practice to explicitly set executable permissions after copying binaries:RUN chmod +x /usr/local/bin/yq /usr/local/bin/orasThis prevents potential permission issues at runtime.
-
Consider adding
--no-cacheor cleaning up yum cache:
The Dockerfile installs packages with yum but does not clean caches, which can increase image size. To optimize image size, add:RUN yum install -y python3 python3-devel mysql-devel && yum clean allOr use
--no-cacheif supported.
Best Practices
-
Add comments describing why these tools are needed:
The current comments just mention "yq and oras tools" and Renovate datasource hints. Adding a short description of why these tools are included (e.g., what CI steps require them) would improve maintainability. -
Test coverage:
The PR description includes images of local test builds, but no automated tests or CI verification are mentioned. Consider adding a minimal smoke test or CI step that verifiesyq --versionandoras versionrun successfully in the built image. This will catch future regressions. -
Label the Dockerfile or image metadata with tool versions:
To improve traceability, you may add labels like:LABEL yq.version="4.52.2" oras.version="1.3.0"
This helps with debugging which versions are included in the image.
Minor Suggestions
- The Renovate datasource comments are helpful; ensure your Renovate config is set to parse and update these correctly for automated version bumps.
No critical issues were found. The PR is a clean, minimal addition that should work well with the suggested improvements.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
…sh from OCI (#4279) Depends on PingCAP-QE/artifacts#905 test result: https://prow.tidb.net/view/gs/prow-tidb-logs/pr-logs/pull/pingcap-inc_tici/929/pull-e2e/2028734174122217472 <img width="823" height="444" alt="image" src="https://github.com/user-attachments/assets/240859c9-2c04-49ad-9b54-d71add981e29" /> <img width="873" height="537" alt="image" src="https://github.com/user-attachments/assets/2636ecc7-67d5-4ab2-8af9-5e2ca2457dd5" /> --------- Signed-off-by: lyb <yebin.li@pingcap.com>
2 participants
local test docker build:
