Repository navigation
ci: cache tool downloads of the container tests in a local proxy - #7670
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <michael.kriese+claude-code@mend.io>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds an Nginx download proxy and integrates it into the distro, base-arm64, and lang build jobs. The jobs collect cache statistics and report proxy errors after failures. The Node.js testp stage clears ChangesDownload proxy
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant BuildJob
participant DownloadProxyAction
participant Nginx
participant NodeDistribution
BuildJob->>DownloadProxyAction: invoke local action
DownloadProxyAction->>Nginx: configure and start proxy
DownloadProxyAction->>Nginx: check Node.js distribution index
Nginx->>NodeDistribution: forward allowlisted request
NodeDistribution-->>Nginx: return response
Nginx-->>DownloadProxyAction: return check response
Merge Risk: ⚪ Minimal · up to The proxy is wired into the intended container tests, with no established issue requiring a fix before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
viceice
marked this pull request as ready for review
October 8, 2026 15:36
PR Summary by QodoCache container-test tool downloads through a runner-local proxy
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Cached builds hide download hosts
|
Member
Author
|
Re the relative redirect finding: that's intended. A redirect is only followed when its (Comment by Claude Opus 5.5 in Claude Code on behalf of @viceice.) |
Co-Authored-By: Claude Opus 5.5 <michael.kriese+claude-code@mend.io>
Co-Authored-By: Claude Opus 5.5 <michael.kriese+claude-code@mend.io>
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 <michael.kriese+claude-code@mend.io>
viceice
enabled auto-merge
October 8, 2026 16:47
Comment on lines
+35
to
+39
|
|
||
| echo "" | ||
| echo "### Download proxy hosts" | ||
| echo "" | ||
| echo "| Host | Requests | Cache hits | Redirects to |" |
There was a problem hiding this comment.
1. Cached builds hide download hosts 📎 Requirement gap ◔ Observability
stats.sh prints Download proxy hosts from the current access log without warning that cached build steps generate no proxy requests. When Bake reuses an image layer, its download hosts are absent from the table, so someone using the summary to plan network allowlisting may miss hosts needed for a clean build.
Agent Prompt
## Issue description
The new host table does not explain that cached build steps can omit hosts needed by a clean build.
## Fix Focus Areas
- .github/actions/download-proxy/stats.sh[35-40]
## Recommended Fix
Add a visible note beside the host table stating that it reflects only requests observed by the proxy in this job and that cached build steps can leave required hosts out. Keep the note in both the job log and summary.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Oct 8, 2026
Member
Author
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.
Changes
The container tests now download tools through a caching proxy on the runner, so stages and bake retries in a job share their downloads:
.github/actions/download-proxy: starts nginx as a caching reverse proxy and setsCONTAINERBASE_CDN=http://host.docker.internal:8099, which the builds already pass throughdocker-bake.hcl. The CLI rewriteshttps://<host>/<path>to<cdn>/<host>/<path>, the proxy forwards it upstream and caches the response; redirects are followed inside the proxy, so GitHub release assets are cached too.squid-deb-proxy) it only accepts the runner and the docker networks, and only fetches from a fixed list of download hosts (allowed-hosts.map), checked for every redirect hop as well; TLS is verified.distro,base-arm64andlang(after the base image build, so its cache key stays the same as inbase);baseand the release job don't use it. On failure, a step prints the proxy's rejected and failed requests, and every run adds the cache hits and misses to the job summary.URL_REPLACE_*unsets the CDN to keep testing the url replacements.Context
Please select one of the following:
Related: #972 (uses the same
CONTAINERBASE_CDNrewrite, and follows redirects inside the proxy) and #7 (the proxy log lists the hosts the test builds download from).AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
Written by Claude Opus 5.5 in Claude Code.
Use of AI in replying to PR comments
Who answers review comments:
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The nginx config was tested locally in docker: GitHub redirect chains, encoded paths (kustomize's
%2F),dl.k8s.io→cdn.dl.k8s.io, cache hits,403for other hosts, and aninstall-tool nodethroughCONTAINERBASE_CDN. The first full CI run passed with the proxy in all docker test jobs on both architectures, so no download host is missing.🤖 Generated with Claude Code
Summary by CodeRabbit