Repository navigation
fix: set UV_NO_SYNC so uv run can't silently revert deployment overrides - #215
Merged
Merged
Conversation
Even after #214's --no-deps and testing-sync fixes, a rebuilt master image still shipped the pre-override openedx-forum (0.4.3, no direct_url.json) rather than the git-installed 0.4.5 the manifest override asked for -- verified by pulling the pushed image directly and inspecting its dist-info. Root cause is a third occurrence of the same bug class, from a source outside this codebase entirely: master edx-platform's own package.json runs `compile-sass` as `uv run --active python scripts/compile_sass.py`. `uv run` resyncs the active venv against pyproject.toml/uv.lock before running anything unless told not to, and master's pyproject.toml declares openedx-forum -- so build_static_assets()'s `npm run compile-sass` reverts the override AGAIN, after both the overrides step and the --no-deps editable install had already left the correct version in place. release/verawood's compile-sass script is a plain `scripts/compile_sass.py`, no `uv run`, which is why that release line never showed this and why it wasn't caught by #214's review. Reproduced directly against uv: `uv run --active python -c ...` on a venv holding an off-lock package reverts it; `UV_NO_SYNC=1 uv run --active ...` does not. `UV_NO_SYNC` has no effect on `uv sync`/`uv pip install`, so it is safe to set unconditionally without touching the explicit `_edx_base_deps_script()` sync or any other uv subcommand this pipeline runs. Set in two places because env vars don't survive the multi-stage copy: install_deps() protects check_deployment() and _prepare_test_run(), which chain directly off it; build_platform()'s clean-base reconstruction (only directories carry across from the throwaway `deps` container, not env vars) needs its own copy to protect collected()/build_static_assets()/docker_image(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation addresses both container paths correctly; only a minor comment wording issue remains.
Pull request overview
Prevents implicit uv run synchronization from reverting deployment package overrides.
Changes:
- Sets
UV_NO_SYNC=1during dependency installation. - Reapplies it after clean-base reconstruction.
File summaries
| File | Description |
|---|---|
src/lehrer/core/platform.py |
Preserves overrides across dependency checks and asset compilation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses review feedback on #215. The clause "of a package master's pyproject.toml declares" was left in twice from an earlier edit that renamed mitxonline to a generic "a package" to satisfy the core-boundary check, without removing the sentence it was duplicating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW
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
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.
What are the relevant tickets?
N/A. Follow-up to #214.
Description (What does it do?)
Sets
UV_NO_SYNC=1on the container in two places souv runcan't silently revert a deployment override.After #214 merged and the three master-line CI images rebuilt, the deployed image still shipped
openedx-forum==0.4.3with nodirect_url.json— the pre-override package, not the git-installed0.4.5the manifest asks for. Verified by pulling the exact digest the build pushed and inspecting it directly, bypassing Kubernetes entirely:This is a third occurrence of the bug class #214 fixed, from a source outside this codebase: master edx-platform's own
package.jsonrunscompile-sassasuv run --active python scripts/compile_sass.py.uv runresyncs the active venv againstpyproject.toml/uv.lockbefore running anything, unless told not to — and master'spyproject.tomldeclaresopenedx-forum. Sobuild_static_assets()'snpm run compile-sassreverts the override again, after both the overrides step and #214's--no-depseditable install had already left the correct version in place.release/verawood'scompile-sassscript is a plainscripts/compile_sass.py, nouv run:— which is why that release line never showed this and #214's review didn't catch it.
Set in two places because environment variables don't survive the multi-stage copy pattern in
build_platform():install_deps()— protectscheck_deployment()and_prepare_test_run(), which chain directly off its returned container.build_platform()'s clean-base reconstruction — only/openedx/venv,/openedx/edx-platformand/openedx/nodeenvare copied across from the throwawaydepscontainer into a freshapt_base(); environment variables are not, socollected()/build_static_assets()/docker_image()need their own copy of the same env var.How can this be tested?
Reproduced the mechanism directly against uv, isolated from the real build:
uv sync --helpanduv run --helpconfirmUV_NO_SYNConly affectsuv run's implicit sync ([env: UV_NO_SYNC=]on--no-sync);uv synchas no such flag, so this cannot interfere with_edx_base_deps_script()'s explicit sync or any otheruv sync/uv pip installcall in this pipeline.uv run pytest tests— 340 passed.pre-commit runclean (ruff, mypy, core-boundary check).Not yet re-verified end-to-end against a real image rebuild — that's the next step, and the one that actually closes this out: rebuild
build-master-mitxonline-edxapp-image(or any master cell) after this merges, then re-run the samepip show openedx-forumcheck above and confirmVersion: 0.4.5with adirect_url.jsonpresent.Additional Context
install_deps()and the clean-base construction return realdagger.Containerobjects, and nothing in this suite exercises those without a live dagger session — consistent with how the rest of this file is tested (only the pure string-returning helpers like_edx_base_deps_script()have unit tests). Flagging rather than forcing a test that would need a new mocking harness.uv run(oruv sync) calls in master'spackage.jsonthat this build path exercises: onlycompile-sass/compile-sass-devuseuv run, and onlycompile-sass(production) runs inbuild_static_assets().postinstall'sscripts/copy-node-modules.shhas neither._prepare_test_run()doesn't reset to a clean base (it chains straight offinstall_deps()), so it picks upUV_NO_SYNCfor free from the first placement. Its own suite doesn't runnpm run compile-sass, so this wasn't otherwise exercising the bug, but there's no reason to leave it unprotected.🤖 Generated with Claude Code
https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW