feat(ci): add docs PDF runtime image#976
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 introduces a new dedicated runtime image for "docs merged-update" under dockerfiles/ci/docs-merged-update. It integrates this image as a separate module ci-docs-merged-update in skaffold and extends both the pull and release CI workflows to build and publish this image independently. The approach is consistent with existing CI runtime image practices, leveraging multi-arch builds with skaffold and GitHub Actions. Overall, the changes are well-structured and align with current CI/CD patterns, with clear separation of concerns and appropriate caching/credentials handling.
Critical Issues
- No critical bugs or security issues detected. The Dockerfile pins base images by digest, and workflows use standard secure actions with GitHub tokens.
Code Improvements
-
Dockerfile: Reduce image layers and improve caching (dockerfiles/ci/docs-merged-update/Dockerfile lines 9-15)
Currently, the Dockerfile does:RUN set -eux; \ export DEBIAN_FRONTEND=noninteractive; \ apt-get update; \ apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei; \ rm -rf /var/lib/apt/lists/*; \ fc-cache -fv >/dev/null || trueThis can be improved by combining
apt-get updateandinstallin oneRUNstep to leverage Docker layer caching better, e.g.:RUN set -eux; \ export DEBIAN_FRONTEND=noninteractive; \ apt-get update && \ apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei && \ rm -rf /var/lib/apt/lists/*; \ fc-cache -fv >/dev/null || trueThis reduces intermediate layers and avoids cache busting.
-
GitHub Actions workflow: unify caching keys and restore keys for skaffold cache (pull & release workflows)
In.github/workflows/pull-ci-runtime-images.yamland.github/workflows/release-ci-runtime-images.yaml, caching keys differ slightly between build and publish steps (e.g.,skaffold/${{ matrix.platform }}/ci/${{ matrix.module }}-${{ github.sha }}vsskaffold/ci/${{ matrix.module }}-${{ github.sha }}). For consistency and better cache hits, align the key format across build and publish jobs.
Suggested:key: skaffold/${{ matrix.platform || 'linux/amd64' }}/ci/${{ matrix.module }}-${{ github.sha }} restore-keys: | skaffold/${{ matrix.platform || 'linux/amd64' }}/ci/${{ matrix.module }}- skaffold/${{ matrix.platform || 'linux/amd64' }}/ci/
This ensures cache reuse across jobs/platforms if applicable.
-
GitHub Actions workflow: add explicit
fail-fast: falsein strategy if parallel jobs are independent
If the skaffold build matrix jobs should continue independently on failure, consider addingfail-fast: falseto the matrix strategy to avoid premature job cancellation. -
GitHub Actions workflow: add
idto checkout steps for better referencing
Addingid: checkoutto checkout steps can improve logs readability and enable future step dependencies if needed.
Best Practices
-
Documentation: Add comments to new skaffold.yaml section (dockerfiles/ci/skaffold.yaml lines 77-100)
The new skaffold config forci-docs-merged-updatelacks comments explaining its purpose or usage. Adding a brief comment block describing the image, platforms targeted, and its role in CI would help future maintainers. -
Testing: No explicit test or validation step for the new image build
The PR description mentions manual validation but no automated test or smoke test in the workflows. Consider adding a lightweight container run test step in the workflow, e.g., runningdocs-merged-update-runtimeimage with--versioncommands or a simple script to verify runtime dependencies. -
Naming: Docker image name consistency
In skaffold.yaml and workflows, the image is nameddocs-merged-update-runtime(skaffold.yaml) but referenced asdocs-merged-updatein workflows and Docker build commands. Align naming to avoid confusion, e.g., always usedocs-merged-update-runtimeordocs-merged-update. -
Style: Align indentation in workflow YAML
In.github/workflows/pull-ci-runtime-images.yamland.github/workflows/release-ci-runtime-images.yaml, some added blocks have slightly inconsistent indentation (spaces vs tabs or levels). Ensure consistent YAML indentation (2 spaces per level) for readability.
Summary of Key Suggestions
- Improve Dockerfile RUN command layering for better caching.
- Standardize skaffold cache keys across workflows.
- Add comments to skaffold.yaml describing new config.
- Add automated smoke test step in CI workflows for the new image.
- Align Docker image naming conventions (
docs-merged-update-runtimevsdocs-merged-update). - Ensure consistent YAML indentation in workflows.
Implementing these will improve maintainability, caching efficiency, and reliability of the new docs merged-update runtime image integration.
![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 introduces a new Dockerfile and Skaffold configuration for a documentation merging and updating runtime. The feedback suggests explicitly installing the fontconfig package to ensure fc-cache works correctly and removing error suppression to catch potential failures during the font cache update. Additionally, there is a recommendation to reconsider the ENTRYPOINT configuration, as the current login shell approach may restrict how arguments are passed to the container.
| RUN set -eux; \ | ||
| export DEBIAN_FRONTEND=noninteractive; \ | ||
| apt-get update; \ | ||
| apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei; \ | ||
| rm -rf /var/lib/apt/lists/*; \ | ||
| fc-cache -fv >/dev/null || true |
There was a problem hiding this comment.
The fc-cache command requires the fontconfig package, which should be explicitly installed to ensure the font cache is correctly updated. Additionally, removing || true ensures that the build fails if font registration fails, which is important for the correctness of the documentation rendering environment.
RUN set -eux; \
export DEBIAN_FRONTEND=noninteractive; \
apt-get update; \
apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei fontconfig; \
rm -rf /var/lib/apt/lists/*; \
fc-cache -fv >/dev/null
| rm -rf /var/lib/apt/lists/*; \ | ||
| fc-cache -fv >/dev/null || true | ||
|
|
||
| ENTRYPOINT ["/bin/bash", "-lc"] |
There was a problem hiding this comment.
Using ["/bin/bash", "-lc"] as the ENTRYPOINT forces the shell to treat the first argument passed to the container as a single command string. This prevents passing multiple arguments naturally (e.g., docker run image ls -la would fail or behave unexpectedly). If a login shell is required to source profiles, consider if this restriction is acceptable for all use cases or if a more flexible entrypoint should be used.
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 introduces a new dedicated docs merged-update runtime image under dockerfiles/ci/docs-merged-update and integrates it as a separate module ci-docs-merged-update in the Skaffold configuration and CI workflows. The update extends both pull request and release GitHub Actions workflows to build and publish this new runtime independently. The PR is well-structured, with clear modular separation and proper CI integration, though some improvements to maintainability, naming consistency, and minor workflow redundancies could be addressed.
Critical Issues
- Missing
dockerfiles/ci/docs-merged-update/Dockerfile
The PR adds CI workflow support for thedocs-merged-updateruntime image, but the Dockerfile for this new image is not included in the diff. Without the Dockerfile, the build will fail.
Location:dockerfiles/ci/docs-merged-update(missing)
Suggestion: Add the Dockerfile for thedocs-merged-updateruntime image, or clarify if it is intentionally omitted.
Code Improvements
-
Duplicate Skaffold Metadata and Config in
dockerfiles/ci/skaffold.yaml
The PR adds a new Skaffold config forci-docs-pdf-runtime(different fromdocs-merged-update) at the end of the file, but the PR title and description focus ondocs-merged-update. This looks like an unrelated or misplaced addition.
Location:dockerfiles/ci/skaffold.yamllines ~78-102
Issue: Mixing concerns in this PR may confuse reviewers and maintainers.
Suggestion: Separate theci-docs-pdf-runtimeSkaffold config into a distinct PR or ensure it is intentionally part of this one with proper description. -
Workflow Caching Key Consistency
The caching keys for Skaffold cache in pull and release workflows differ in detail (e.g., platform-specific keys in pull CI, but not in release CI). For consistency and cache hit maximization, unify the cache keys where possible.
Location:.github/workflows/pull-ci-runtime-images.yamllines ~78-97.github/workflows/release-ci-runtime-images.yamllines ~80-109
Suggestion: Consider aligning cache keys, e.g., always include platform and module in keys, or document why differences exist.
-
Use of
ifCondition inskaffold-docs-merged-updateJobs
Theifcondition in both workflows checks fordocs-merged-updatechanges orskaffoldchanges. However, thedetect-changesstep outputs are used differently in pull vs release workflows, and could cause unexpected builds.
Suggestion: Add comments or refactor the condition to clarify logic, possibly extracting to a reusable expression. -
Hardcoded Skaffold Version
The Skaffold version2.16.0is hardcoded in both workflows. If multiple Skaffold modules are managed, consider centralizing this version in a reusable variable or a workflow template to ease upgrades.
Suggestion: Use workflow input variables or a shared action for Skaffold setup.
Best Practices
-
Lack of Comments in Workflow Files
The added jobs and steps in.github/workflows/pull-ci-runtime-images.yamlandrelease-ci-runtime-images.yamllack comments explaining the purpose of each step, especially for less common steps like caching or conditional build logic.
Location:.github/workflows/pull-ci-runtime-images.yamllines 210-275,.github/workflows/release-ci-runtime-images.yamllines 220-285
Suggestion: Add concise comments describing the purpose of each major step for maintainability. -
Naming Consistency:
docs-merged-updatevsdocs-pdf-runtime
The PR introducesdocs-merged-updateruntime image, but the Skaffold config added is forci-docs-pdf-runtimeand the Dockerfile added is fordocs-pdf-runtime. This inconsistency can confuse users and maintainers.
Suggestion: Align naming consistently throughout the PR, or clarify if the PDF runtime is a separate concern. -
Testing Coverage of New Image
The PR mentions local manual tests with container runtime commands, but no automated tests are added for the new runtime image or integration with CI workflows.
Suggestion: Add smoke or integration tests that validate the new runtime image functionality in CI. -
Dockerfile Layer Optimization
In the newdocs-pdf-runtime/Dockerfile, theapt-get updateandapt-get installsteps are combined properly, but the cache rebuild could be optimized by pinning package versions or clearing cache more explicitly. Minor but beneficial for reproducible images.
Suggestion: Consider addingapt-get cleanand pinning versions if image reproducibility is important.
Summary of Actionable Suggestions
- Add the missing Dockerfile for `docs-merged-update` runtime image or clarify its absence.
- Remove or separate the Skaffold config for `ci-docs-pdf-runtime` if unrelated to this PR.
- Align Skaffold cache keys across pull and release workflows for consistency.
- Add comments explaining workflow job steps and conditional logic.
- Use consistent naming across runtime images (`docs-merged-update` vs `docs-pdf-runtime`).
- Consider adding automated tests for the new runtime image and workflow integration.
- Centralize Skaffold version usage in workflows for easier upgrades.
- Optimize Dockerfile layer caching and package version pinning if needed.Addressing these points would significantly improve the clarity, maintainability, and robustness of the introduced changes.
wuhuizuo
changed the title
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 introduces a new CI runtime image dedicated to building docs PDFs, incorporating pandoc, xelatex, rclone, and WenQuanYi fonts. The image is added under dockerfiles/ci/docs-pdf-runtime and integrated into the CI workflows for pull requests and releases using skaffold as its own module. The implementation is comprehensive, covering Dockerfile, skaffold config, and GitHub Actions workflows with multi-arch support. The overall quality is solid, with clear structuring and validation steps.
Critical Issues
-
Dockerfile base image pinning inconsistency
File:dockerfiles/ci/docs-pdf-runtime/Dockerfile
The base image uses a specific sha256 digest, which is good for reproducibility. However, theCOPY --from=rclone/rclone@sha256:...also uses a digest. Make sure this referenced digest is up to date and corresponds to a stable rclone release to avoid unexpected breakage or security issues. -
Potential missing multi-arch support for
rclonebinary
File:dockerfiles/ci/docs-pdf-runtime/Dockerfilelines 6-7
The rclone binary is copied from a specific image by digest, but it’s unclear if this binary supports both amd64 and arm64 architectures. Since the image is built for both platforms, this could cause runtime failures on arm64.
Suggestion: Use a multi-arch compatible approach to install rclone, e.g., download the binary per architecture or use a multi-arch base image that includes rclone.
Code Improvements
-
Skaffold config: use consistent module names
File:dockerfiles/ci/skaffold.yamllines 77-102
The new module is namedci-docs-pdf-runtimein the workflows, but the image in skaffold isdocs-pdf-runtime. It’s better to keep consistent naming to avoid confusion: either rename the skaffold config’smetadata.nameor align the image name accordingly.
Suggestion: Use the same module/image name in skaffold and workflows, e.g.:metadata: name: ci-docs-pdf-runtime build: artifacts: - image: ci-docs-pdf-runtime
-
Reduce duplication in GitHub Actions workflows
Files:.github/workflows/pull-ci-runtime-images.yamland.github/workflows/release-ci-runtime-images.yaml
The new jobsskaffold-docs-pdf-runtimein both workflows are very similar. Consider extracting common steps into reusable workflow files or composite actions to DRY up code and ease maintenance. -
Dockerfile optimization: combine RUN commands and reduce layers
File:dockerfiles/ci/docs-pdf-runtime/Dockerfileline 9
Theapt-get updateandapt-get installare combined properly, but thefc-cachecommand is run separately without&&. This causes an extra layer and might mask errors.
Suggestion: Combine into one RUN block with proper error handling, for example:RUN set -eux; \ export DEBIAN_FRONTEND=noninteractive; \ apt-get update && \ apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei && \ rm -rf /var/lib/apt/lists/* && \ fc-cache -fv -
Add explicit healthcheck or runtime test step (optional)
To ensure the runtime image is functional (pandoc, xelatex, rclone, fonts), consider adding a lightweight test script or healthcheck in the Dockerfile or as part of the CI build steps.
Best Practices
-
Add comments in Dockerfile explaining key steps
File:dockerfiles/ci/docs-pdf-runtime/Dockerfile
Adding brief comments describing why pandoc/extra is chosen, why rclone is copied this way, and the purpose of WenQuanYi fonts would improve maintainability. -
Expand documentation in skaffold.yaml
File:dockerfiles/ci/skaffold.yaml
Adding comments to explain the new module and its purpose helps future maintainers understand the config at a glance. -
Add testing or validation step in workflows
The PR mentions manual validation of the image by running commands inside the container. It would be beneficial to automate these checks inside the CI workflow to catch regressions early. -
Use consistent naming in GitHub workflow permissions
The workflows setcontents: readandpackages: writepermissions. Confirm that these are the minimal required permissions per GitHub best practices to reduce risk. Ifcontents: readis not needed, remove it.
Summary of actionable suggestions
- Verify that the rclone binary used in the Dockerfile supports all target architectures; consider installing rclone per-arch instead of copying a static binary.
- Align skaffold module and image names (`ci-docs-pdf-runtime` vs `docs-pdf-runtime`) for consistency.
- Refactor GitHub Actions workflows to reuse common steps for building and publishing this image.
- Combine Dockerfile RUN commands into a single layer with proper chaining and error handling:
RUN set -eux; \
export DEBIAN_FRONTEND=noninteractive; \
apt-get update && \
apt-get install -y --no-install-recommends ca-certificates fonts-wqy-microhei && \
rm -rf /var/lib/apt/lists/* && \
fc-cache -fv
- Add comments to Dockerfile and skaffold configuration explaining design decisions.
- Consider adding automated runtime validation tests in the CI workflow for the docs PDF runtime image.
- Review and minimize GitHub workflow permissions to follow least privilege principle.Implementing these will improve the robustness, maintainability, and clarity of this new CI runtime image integration.
1 participant
Summary
dockerfiles/ci/docs-merged-updateci-docs-merged-updatemoduleValidation
dockerfiles/ci/skaffold.yaml,.github/workflows/pull-ci-runtime-images.yaml, and.github/workflows/release-ci-runtime-images.yamldocker build -t docs-merged-update:test -f dockerfiles/ci/docs-merged-update/Dockerfile dockerfiles/cidocker run --rm --entrypoint /bin/bash docs-merged-update:test -lc 'python3 --version; pandoc --version | head -n 1; xelatex --version | head -n 1; rclone version | head -n 1; fc-match "WenQuanYi Micro Hei"; fc-match "WenQuanYi Micro Hei Mono"'Note
docker buildxCLI component installed; GitHub Actions usessetup-buildx-actionfor the PR/release path