refactor(packaging): Use the shared ca-trust library from yscope-dev-utils. - #47
Open
jackluo923 wants to merge 1 commit into
Open
refactor(packaging): Use the shared ca-trust library from yscope-dev-utils.#47jackluo923 wants to merge 1 commit into
jackluo923 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe build scripts add optional ChangesHost CA certificate build integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BuildScripts
participant build_image
participant DependencyImage
participant NetworkTools
User->>BuildScripts: Enable --with-ca-certs
BuildScripts->>build_image: Request dependency-image build
build_image->>DependencyImage: Pass staged CA-trust context
DependencyImage->>NetworkTools: Mount CA trust for network steps
NetworkTools-->>DependencyImage: Install packages and dependencies
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/build-packages/dependency-image/Dockerfile`:
- Around line 12-21: Update build_image to validate the CA-trust mount contract
before invoking the Docker build: stage the host bundle locally using
CA_TRUST_DIR and CA_TRUST_BUNDLE_FILENAME from yscope-dev-utils, then fail if
the generated staged directory or bundle path differs from the Dockerfile’s
CA_TRUST_DIR and CA_TRUST_BUNDLE values. Reuse the validation pattern from
build-packages.sh and preserve the existing --with-ca-certs flow.
- Line 10: Update the Dockerfile’s manylinux base image reference from the
mutable latest tag to a specific immutable image digest, preserving the
manylinux_2_28 image while ensuring future builds resolve the same base layers
until deliberately reviewed and updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b23c269-f8df-4edd-9637-2ff54704b60e
📒 Files selected for processing (13)
tools/build-packages/README.mdtools/build-packages/build-dependency-image.shtools/build-packages/build-packages.shtools/build-packages/dependency-image/Dockerfiletools/build-packages/dependency-image/use-host-ca.shtools/build-packages/dependency-image/utils.shtools/build-packages/internal/ca-trust/README.mdtools/build-packages/internal/ca-trust/container.shtools/build-packages/internal/ca-trust/generators/java-pkcs12/README.mdtools/build-packages/internal/ca-trust/generators/java-pkcs12/generate.shtools/build-packages/internal/ca-trust/host.shtools/build-packages/internal/container/build-artifacts.shtools/yscope-dev-utils
💤 Files with no reviewable changes (6)
- tools/build-packages/dependency-image/use-host-ca.sh
- tools/build-packages/internal/ca-trust/host.sh
- tools/build-packages/internal/ca-trust/generators/java-pkcs12/generate.sh
- tools/build-packages/internal/ca-trust/generators/java-pkcs12/README.md
- tools/build-packages/internal/ca-trust/container.sh
- tools/build-packages/internal/ca-trust/README.md
jackluo923
force-pushed
the
refactor/ca-certs
branch
from
August 2, 2026 18:53
484b2c7 to
3749530
Compare
jackluo923
changed the title
jackluo923
force-pushed
the
refactor/ca-certs
branch
from
August 2, 2026 22:32
3749530 to
19b61f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tools/build-packages/dependency-image/Dockerfile (1)
10-10:⚠️ Potential issue | 🟠 MajorPin the manylinux base image.
quay.io/pypa/manylinux_2_28:latestremains mutable. Pin a reviewed digest so rebuilds use the same base layers. Docker documents tags as mutable and digests as immutable. (docs.docker.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/build-packages/dependency-image/Dockerfile` at line 10, Update the Dockerfile’s manylinux base image reference from the mutable latest tag to a reviewed immutable digest, retaining the manylinux_2_28 image and ensuring rebuilds resolve the same base layers.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/build-packages/dependency-image/utils.sh`:
- Around line 106-123: Move the ensure_yscope_dev_utils_submodule call below the
with_ca_certs branch in the dependency-image build flow, so non-CA builds return
without initializing tools/yscope-dev-utils while CA-enabled builds still
initialize it before continuing.
In `@tools/build-packages/internal/container/build-artifacts.sh`:
- Line 36: Update the yscope-dev-utils submodule gitlink and checkout to the
merged commit from PR `#119`, then verify that
exports/docker/ca-trust/container.sh exists at that revision before retaining
the source call in the build script.
---
Duplicate comments:
In `@tools/build-packages/dependency-image/Dockerfile`:
- Line 10: Update the Dockerfile’s manylinux base image reference from the
mutable latest tag to a reviewed immutable digest, retaining the manylinux_2_28
image and ensuring rebuilds resolve the same base layers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30c0c0ba-0ab2-4210-aca5-825bf79652b1
📒 Files selected for processing (13)
tools/build-packages/README.mdtools/build-packages/build-dependency-image.shtools/build-packages/build-packages.shtools/build-packages/dependency-image/Dockerfiletools/build-packages/dependency-image/use-host-ca.shtools/build-packages/dependency-image/utils.shtools/build-packages/internal/ca-trust/README.mdtools/build-packages/internal/ca-trust/container.shtools/build-packages/internal/ca-trust/generators/java-pkcs12/README.mdtools/build-packages/internal/ca-trust/generators/java-pkcs12/generate.shtools/build-packages/internal/ca-trust/host.shtools/build-packages/internal/container/build-artifacts.shtools/yscope-dev-utils
💤 Files with no reviewable changes (6)
- tools/build-packages/dependency-image/use-host-ca.sh
- tools/build-packages/internal/ca-trust/generators/java-pkcs12/README.md
- tools/build-packages/internal/ca-trust/container.sh
- tools/build-packages/internal/ca-trust/generators/java-pkcs12/generate.sh
- tools/build-packages/internal/ca-trust/host.sh
- tools/build-packages/internal/ca-trust/README.md
jackluo923
force-pushed
the
refactor/ca-certs
branch
3 times, most recently
from
August 2, 2026 23:24
b180de7 to
891ce53
Compare
…utils.
This repo carried the original ca-trust library under
tools/build-packages/internal/, and a second, independent implementation of the
same idea for the `docker build` path. yscope-dev-utils now hosts the library
and has grown the build-time half it was missing, so both can go.
Delete the vendored copy and source the submodule instead. The vendored files
were functionally identical to the upstream ones (container.sh and generate.sh
byte-for-byte; host.sh differed only by a comment), so the `docker run` path
behaves exactly as before. Use `ca_trust_add_run_args` for the mount/env flags,
the renamed `ca_trust_stage_host_bundle`, and `${CA_TRUST_BUNDLE_FILENAME}` in
place of the hardcoded filename, which had been a silent coupling to the
library's internals.
Converge the dependency-image build onto the same library: drop
`_stage_host_ca_bundle` and `use-host-ca.sh` -- a hand-rolled duplicate of
`container.sh`'s export block -- and mount the library's staging directory
instead. The two paths now agree on the mount point, the bundle filename, and
the set of environment variables exported; previously the build path used
/run/secrets/host-ca with a file named `host-ca`.
Both sites also use `ca_trust_stage_or_fail` rather than repeating the
stage-then-check pair. `ca_trust_stage_host_bundle` tolerates an empty bundle on
purpose -- a build with no host CA context is normal -- so every caller that
explicitly asked for CA trust had grown the same follow-up check. That pair was
duplicated four times across this repo and y-scope/clp; it now lives in the
library.
Make the dependency-image build opt-in via `--with-ca-certs`, matching
build-packages.sh, and forward the flag from build-packages.sh so the two stay
consistent. That build previously staged the host bundle unconditionally, so
without the forwarding a corporate-gateway developer with no cached or pullable
build-env image would fail inside the image build before the flag had any
effect. The Dockerfile declares `FROM scratch AS ca_trust` as an empty default,
so an unprovided context resolves to an empty mount rather than failing --
BuildKit otherwise tries to pull the context name as an image -- and CI now
passes nothing at all.
Source the library lazily, at the point of use. It lives in a submodule that
this script only initializes partway through, so sourcing it at the top broke
every invocation on a fresh clone, `--help` included.
Verified against the real manylinux_2_28 base in both modes: with no context the
image keeps its own SSL_CERT_FILE and dnf runs without an sslcacert override;
with --with-ca-certs both point at the staged bundle. dnf installs succeeded
either way.
Co-Authored-By: Claude <noreply@anthropic.com>
jackluo923
force-pushed
the
refactor/ca-certs
branch
from
August 11, 2026 00:56
891ce53 to
61820fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/yscope-dev-utils`:
- Line 1: Update the yscope-dev-utils gitlink to reference the merged commit
containing the required helper after pull request `#119` is merged, replacing the
current unmerged commit reference 060c45c24f2bcd92e4af69b9360f253edc11ba80.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 281209c7-1762-4eff-a311-88e1aeaa2022
📒 Files selected for processing (2)
tools/build-packages/README.mdtools/yscope-dev-utils
| @@ -1 +1 @@ | |||
| Subproject commit 0c214c44acddff330a204201428ea145e641891d | |||
| Subproject commit 060c45c24f2bcd92e4af69b9360f253edc11ba80 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
expected="060c45c24f2bcd92e4af69b9360f253edc11ba80"
actual="$(git rev-parse HEAD:tools/yscope-dev-utils)"
test "${actual}" = "${expected}"
git -C tools/yscope-dev-utils cat-file -e "${expected}^{commit}"
git -C tools/yscope-dev-utils show "${expected}:exports/docker/ca-trust/host.sh" >/dev/null
git -C tools/yscope-dev-utils for-each-ref \
--contains "${expected}" \
--format='%(refname)' \
refs/remotesRepository: y-scope/clp-plugin-presto-connector
Length of output: 349
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- gitlink ---'
git rev-parse HEAD:tools/yscope-dev-utils
printf '%s\n' '--- submodule configuration ---'
git config --file .gitmodules --get-regexp 'submodule\..*\.(path|url)' || true
git -C tools/yscope-dev-utils remote -v || true
printf '%s\n' '--- relevant remote refs ---'
url="$(git config --file .gitmodules --get submodule.tools/yscope-dev-utils.url)"
git ls-remote "$url" \
refs/heads/main \
refs/heads/master \
refs/pull/119/head \
refs/pull/119/merge \
'refs/tags/*' | grep -E 'refs/(heads/(main|master)|pull/119/(head|merge)|tags/)' || trueRepository: y-scope/clp-plugin-presto-connector
Length of output: 774
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
repo='y-scope/yscope-dev-utils'
sha='060c45c24f2bcd92e4af69b9360f253edc11ba80'
printf '%s\n' '--- pull request metadata ---'
curl -fsSL "https://api.github.com/repos/${repo}/pulls/119" |
jq '{state, merged, merge_commit_sha, base: .base.ref, head: .head.ref, head_sha: .head.sha}'
printf '%s\n' '--- target commit ---'
curl -fsSL "https://api.github.com/repos/${repo}/commits/${sha}" |
jq '{sha, message: .commit.message, parents: [.parents[].sha]}'
printf '%s\n' '--- pull request commits ---'
curl -fsSL "https://api.github.com/repos/${repo}/pulls/119/commits?per_page=100" |
jq '[.[] | {sha, message: .commit.message}]'
printf '%s\n' '--- commit ancestry comparison ---'
curl -fsSL "https://api.github.com/repos/${repo}/compare/main...${sha}" |
jq '{status, ahead_by, behind_by, commits: [.commits[].sha]}'
printf '%s\n' '--- exported helper ---'
curl -fsSL "https://raw.githubusercontent.com/${repo}/${sha}/exports/docker/ca-trust/host.sh"Repository: y-scope/clp-plugin-presto-connector
Length of output: 12089
Update the gitlink after yscope-dev-utils#119 merges. Commit 060c45c24f2bcd92e4af69b9360f253edc11ba80 contains the required helper, but pull request #119 is still open. The gitlink does not reference a merged commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/yscope-dev-utils` at line 1, Update the yscope-dev-utils gitlink to
reference the merged commit containing the required helper after pull request
`#119` is merged, replacing the current unmerged commit reference
060c45c24f2bcd92e4af69b9360f253edc11ba80.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Why
Corporate networks often run a TLS-inspecting gateway that re-signs HTTPS traffic with the company's own certificate authority. Your laptop trusts it; a container doesn't. Builds then fail with certificate errors that give no hint of the cause, so each project invents its own fix —
y-scope/clpandy-scope/clp-plugin-presto-connectorboth had one, and they agreed on almost nothing.Ours was the best of them, so it now lives in yscope-dev-utils#119, generalized for
y-scope/clptoo. This PR deletes our copy and uses the shared one, so that fixes land in one place.What changed
The vendored files were functionally identical to the upstream ones (
container.shandgenerate.shbyte-for-byte;host.shdiffered only by a comment), so thedocker runpath behaves exactly as before.Converging the dependency-image build onto the same library drops
_stage_host_ca_bundleanduse-host-ca.sh— a hand-rolled duplicate ofcontainer.sh's export block. The two paths now agree on the mount point, the bundle filename, and the variables exported; previously the build path used/run/secrets/host-cawith a file namedhost-ca.That build is now opt-in via
--with-ca-certs, matchingbuild-packages.sh, which also forwards the flag. It previously staged the host bundle unconditionally.Depends on yscope-dev-utils#119. The submodule points at that branch; it needs re-pointing to the merged SHA before this merges.
Breaking changes
None.
--with-ca-certskeeps its meaning and default (off), and thedocker runpath is unchanged.Validation performed
Verified against the real
manylinux_2_28base in both modes: with no context the image keeps its ownSSL_CERT_FILEand dnf runs without an sslcacert override; with--with-ca-certsboth point at the staged bundle. dnf installs succeeded either way, and the named context correctly overrides the emptyFROM scratch AS ca_truststage.Fixed three defects that made the feature inert:
CA_TRUST_JVMnever reached the container (fixed upstream in #119), so Maven silently got no truststore while curl/git/pip worked.build-packages.sh --with-ca-certsdidn't forward the flag to the dependency-image build, so a corporate-gateway developer with no cached image failed inside the image build.build-packages.shbroke every invocation on a fresh clone,--helpincluded, since submodules are only initialised partway through. Confirmed fixed against an uninitialised checkout.Checklist
Summary by CodeRabbit
--with-ca-certsbuild option to propagate host certificate trust during dependency-image builds.