build(docker): allow third-party addon repos to be pinned by commit sha - #507
build(docker): allow third-party addon repos to be pinned by commit sha#507reichie020212 wants to merge 5 commits into
Conversation
download_module could only fetch a branch head: it hardcoded the
/archive/refs/heads/<ref>.tar.gz URL, which 404s for a commit sha. The
addon repositories were therefore an unpinned dependency, and two builds
of the same OpenSPP commit could pick up different OCA code.
Give the function a ref that may be a branch, a tag or a commit sha, and
expose one ARG per downloaded repo so a deployment can pin without
editing the Dockerfile:
docker build --build-arg OCA_SERVER_UX_REF=<sha> ...
A sha carries no ref namespace, so it is served from /archive/<sha>.tar.gz
instead; 7 or more hex digits (short or full sha) selects that form and
anything else keeps today's branch URL. Every default is 19.0, so a build
that passes no new argument resolves exactly the same six URLs and
tarball cache paths as before. No repository's default ref is changed
here — which commits to pin is a deployment decision.
The ARGs sit immediately above the RUN that consumes them so that
changing a ref only invalidates the download layer, and because a global
ARG declared before FROM is not in scope inside the stage.
Signed-off-by: Red <redick@newlogic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #507 +/- ##
==========================================
- Coverage 76.92% 76.88% -0.05%
==========================================
Files 735 703 -32
Lines 47935 45739 -2196
==========================================
- Hits 36874 35166 -1708
+ Misses 11061 10573 -488
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
About the three red test checks — they are not caused by this change, and they make its case
Why it happens. Why That cached layer holds a pre-rename snapshot of What the same logs prove about backwards compatibility. In this PR's build the layer — i.e. the pre-change URL, resolved by CI itself, for all six repositories. Two ways forward, both yours to pick. Fix |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Reviewed at ed7e236 (CI green after the branch update picked up the spp_user_roles fix from #508).
Verified
- The diff is exactly
docker/Dockerfile+docker/README.md; the merge commit adds nothing else. - Replayed the ref classifier under
/bin/dashfor 13 refs:19.0,18.0,main,abc,123456→refs/heads/…;deadbee,DEADBEE,1234567and a full sha → bare/archive/<ref>. Cache keys for the six default call sites are unchanged. - Live GitHub:
/archive/19.0and/archive/refs/heads/19.0→ 200;/archive/8e51206(short sha) → 200 and the tarball root isserver-ux-<full sha>/, so--strip-components=1is right;/archive/deadbee→ 404. - Nothing else in the repo references
download_moduleor the old URL;ci.yml,ci-full.ymlandsecurity.ymlbuild with defaults, so no workflow changes are needed.
Must fix before merge
1. Tags are documented but not routed. The README and the Dockerfile comment say a *_REF accepts "a branch, a tag or a commit sha". A tag is non-hex, so it takes the refs/heads/ route and 404s. Checked live on OpenSPP/odoo-job-worker tag 2026.08:
404 /archive/refs/heads/2026.08.tar.gz
200 /archive/refs/tags/2026.08.tar.gz
200 /archive/2026.08.tar.gz
Either drop "tag" from both places, or send every non-branch ref through the bare /archive/<ref>.tar.gz form, which resolves branches, tags and shas alike. A deployer following the README as written would hit the cryptic tar failure below.
Suggestions (non-blocking)
2. curl -fsSL. Without -f, a 404 stores a 14-byte "Not Found" body and the build dies later in tar with not in gzip format. It is not permanent (the gzip -t check re-downloads next build) but the message hides the cause. Pre-existing, but sha pinning makes a mistyped ref more likely, so worth adding here.
3. README example. Under "pin the addons to exact commits", OCA_SERVER_TOOLS_REF=19.0 is the default branch, not a pin. Either drop that line or use a sha.
Observation, out of scope
With a persistent buildkit cache mount, a branch tarball such as server-ux-19.0.tar.gz is reused as long as it is valid gzip, so a long-lived builder never refreshes branch heads. That is the mirror image of the drift this PR fixes; sha pinning sidesteps it. Worth a separate issue.
Happy to approve as soon as (1) is addressed.
Review finding 1 on #507. The Dockerfile comment and docker/README.md both advertised "a branch, a tag or a commit sha", but a tag name is non-hex and so takes the refs/heads route, which 404s: 404 /archive/refs/heads/2026.08.tar.gz 200 /archive/refs/tags/2026.08.tar.gz (OpenSPP/odoo-job-worker, checked live) A deployer following the README as written got a curl 404 body and a later "not in gzip format" failure from tar. Narrow the prose to what the code does and point at the tag's commit sha, which is the reproducible pin anyway. No behaviour change. Signed-off-by: Red <redick@newlogic.com>
Review suggestion 2 on #507. Without -f, a 404 is written to the cache as a 14-byte "Not Found" body and the build dies further down in tar with "not in gzip format", which hides the cause. Replayed under /bin/dash in python:3.13-slim-bookworm with a deliberately bad ref: before: gzip: stdin: not in gzip format / tar: Child returned status 1 RUN exit 2, and a 14-byte tarball left in the /tmp/downloads cache after: curl: (22) The requested URL returned error: 404 RUN exit 22, and nothing cached Pre-existing, but sha pinning makes a mistyped ref likelier, so it is worth having here. Defaults are untouched: the six call sites replayed old-vs-new under dash record byte-identical (tarball, url) pairs, so an existing build cache stays valid. Signed-off-by: Red <redick@newlogic.com>
Review suggestion 3 on #507. Under "pin the addons to exact commits" the second line read OCA_SERVER_TOOLS_REF=19.0, which is the default branch and so demonstrates the opposite of the sentence above it. Use a real OCA/server-tools commit sha (028b450b, 19.0 head at the time of writing; /archive/<sha>.tar.gz checked 200). Signed-off-by: Red <redick@newlogic.com>
|
Thanks — all three addressed at 1. Tags documented but not routed — fixed, by narrowing the proseYou are right, and I took the first of your two options: the routing is unchanged and the claim is now what the code does. Reproduced your check independently before touching anything:
Why not the second option (route everything through the bare 2.
|
| before | after | |
|---|---|---|
| where it fails | tar: gzip: stdin: not in gzip format |
curl: (22) The requested URL returned error: 404 |
| RUN exit | 2 | 22 |
left in the /tmp/downloads cache |
a 14-byte "Not Found" tarball | nothing |
So -f also stops the poisoned entry being written in the first place, rather than relying on the next build's gzip -t to evict it.
3. README example was not a pin — fixed (b8968061)
OCA_SERVER_TOOLS_REF now carries a real sha, 028b450b06ebd70424534cbd89f37ed134f7200d (/archive/<sha>.tar.gz → 200).
Observation (buildkit cache never refreshes a branch tarball) — filed as #510
Out of scope here, agreed, and I think it is the same mechanism as the base_user_role breakage one layer up. Written up with the three options I can see; it needs a decision rather than a patch.
Re-verification after the round
- Defaults still byte-identical. Extracted the pre- and post-change
RUNbodies (comment-only lines stripped the way Docker's parser strips them — confirmed against CI's own echo of the layer) and replayed both under/bin/dashover all six call sites with a stubbedcurlrecording(tarball, url).diffis empty. The only textual difference in the function is the-f. - A real image build now exists, which the original body could not claim: CI's
buildjob re-ran the download layer (#18 [builder 9/10], notCACHED) withcurl -fsSLand the six19.0defaults, green. I have struck the "no fulldocker buildwas run" caveat from the body. - No test suites are in scope — the diff is still exactly
docker/Dockerfile+docker/README.md.
2 participants
Problem
download_moduleindocker/Dockerfilecan only fetch a branch head. The URL ishardcoded to the branch namespace:
GitHub serves a commit sha from
/archive/<sha>.tar.gzand 404s on/archive/refs/heads/<sha>.tar.gz, so there is no way to pass a sha through thisfunction:
The practical consequence: the six third-party addon repositories are an unpinned
dependency of a pinned image. Two builds of the same OpenSPP2 commit, a week apart, can
bake in different
OCA/server-ux,OCA/server-tools,OCA/rest-framework, … code — anda deployment that pins
OPENSPP2_COMMITstill has no way to say which addon commits itwas tested against. Reproducing a build, or bisecting a regression that came from an OCA
change rather than ours, is not currently possible.
Reported by @reichie020212 while pinning an OpenSPP2 build for the DSWD 4Ps deployment:
Change
download_module's third parameter is now a ref: a branch or a commit sha(short or full). 7-or-more hex digits selects
/archive/<ref>.tar.gz; anything elsekeeps today's
/archive/refs/heads/<ref>.tar.gz. A tag name is not accepted — it isnon-hex, so it takes the
refs/heads/route and 404s; pass the commit sha the tagpoints at. (This paragraph and both doc copies said "a branch, a tag or a commit sha"
until review round 1; the code never routed tags, and the prose has been corrected
rather than the routing widened — see finding 1 below.)
One
ARG <REPO>_REF=19.0per downloaded repository, so a deployment can pin withoutediting the Dockerfile:
A hex-only branch name of 7+ characters (
deadbee) would be read as a sha — harmless,because GitHub's bare
/archive/<rev>.tar.gzresolves branch names as well(
/archive/19.0.tar.gz→ 200); the branch form is kept for non-shas only because it isunambiguous between a branch and a like-named tag.
docker/README.mddocuments the arguments under Build.curlgained-fso a badref fails at the download with its 404 instead of later in
tarwithnot in gzip formatover a cached 14-byte error body.No repository's default ref is changed. Every
*_REFdefaults to19.0, which iswhat the call sites resolve today. Which commits to pin is a deployment decision, not
one this PR makes.
The
ARGs sit immediately above theRUNthat consumes them, for two reasons: a globalARGdeclared beforeFROMis not in scope inside a stage (under theRUN'sset -euthat is a hard build failure, not an empty string), and declaring them at the top of the
builder stage would invalidate the apt/uv layers whenever a ref changes.
Backwards compatibility
This is the top requirement, so it is asserted rather than argued: the pre-change and
post-change functions were both extracted from the Dockerfile and replayed over all six
production call sites with a stubbed
curlrecording(tarball, url). The recordings arebyte-identical:
Neither the URLs nor the
/tmp/downloadscache keys move, so an existing build cache isstill valid and a build that passes no new argument downloads exactly what it downloads
today.
Verification
The function body was extracted verbatim from the Dockerfile (comment-only lines
stripped the way Docker's parser strips them) and executed under
/bin/dash— the real/bin/shof thepython:3.13-slim-bookwormbase — in a throwaway container, against thelive GitHub API:
…/archive/refs/heads/19.0.tar.gz, 15 addons extracted19.0…/archive/f2d9a5bc….tar.gz, 14 addons extracted--strip-components=1on a sha tarballserver-ux-<full sha>/even for a short shagzip -tclean19.0,18.0,main,v19.0.1.0.0,abc→ branch;deadbee, full sha, upper-case sha → shaARGscope + overrideARGreaches theRUN(default19.0, and an overridden sha); a globalARGreferenced in-stage aborts the build underset -euTwo things I did not verify, stated plainly:
No full— superseded. CI'sdocker buildwas runbuildjob now builds theimage end-to-end at head: layer
#18 [builder 9/10]runs (notCACHED) withcurl -fsSLand the six default19.0refs, and the job is green(run 34310621689).
The individual exercises below still stand for the sha/branch paths, which a
default build does not reach.
docker buildx build --check --target builderreports no warnings, but a negativecontrol showed it does not catch an out-of-scope
ARG, so it is evidence that thefile parses, not that the scoping is right. The scoping was verified with a separate
minimal build instead.
Known limitation (pre-existing, unchanged)
A ref containing
/(e.g.feature/x) breaks the download cache path, because the ref isinterpolated into the tarball filename
/tmp/downloads/${dest}-${ref}.tar.gz. This istrue before and after this change — verified against both versions of the function — and
no call site uses such a ref, so it is left alone rather than fixed in passing.
ODOO_SOURCE/ODOO_VERSION(the Odoo/OCB tarball a few lines above) is stillbranch-only; extending the same treatment there is deliberately out of scope.
Deploy branch
feat/download-module-commit-sha-pin-on-64e6b31ccarries the same commit cherry-pickedonto
64e6b31c(the commit a downstream deployment is currently pinned to), so thatdeployment can consume this without taking the 111 commits
19.0has since gained. ThisPR is cut from
19.0to keep the review clean;docker/Dockerfileanddocker/README.mdare identical at both bases, so the cherry-pick was conflict-free.
Review round 1 (@gonzalesedwin1123,
ed7e236f)docker/README.mdand this body. Confirmed live:odoo-job-workertag2026.08→ 404 on/archive/refs/heads/, 200 on/archive/refs/tags/. Routing was left alone so no default resolution path moves; a tag is pinnable today as its sha, which is the reproducible form anyway.curl -fsSL/bin/dashwith a bad ref: before →gzip: stdin: not in gzip format, RUN exit 2, 14-byte tarball left in the cache; after →curl: (22) ... 404, RUN exit 22, nothing cached.OCA_SERVER_TOOLS_REFnow carries a real sha (028b450b…, checked 200).Defaults re-verified after the round: the six call sites replayed old-vs-new under
/bin/dashrecord byte-identical(tarball, url)pairs, so build caches stay valid and the only textual change to the function is the-fflag.