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 2 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.
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, a tag, 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.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.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:
docker buildwas run (a shared build host was busy). The function's logic,its URL selection, its extraction and the
ARGplumbing were each exercisedindividually as above; the end-to-end image build was not.
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.