fix(ci): update README sync for source-built stack - #131
Conversation
The version sync workflow still parsed ROCm from FROM and expected the removed VLLM_VERSION and VLLM_ROCM_VARIANT arguments. After the source-build stack landed, the extraction step exited before producing outputs. Read the current Dockerfile ARGs, update the current badge and stack-table layout, fail on missing or ambiguous fields, and exercise the renderer on pull requests.
|
@2imi9 I want to go a different way. I want to add this as a CI check on code on a PR into main and give the user direction on how to fix so we can get merges into main. If you want to adapt your PR to do that I will review that! |
sbates130272
left a comment
sbates130272
left a comment
There was a problem hiding this comment.
Let's change this to trigger on PR into main and run a check and inform user of any mismatches they need to fix before merge.
|
@sbates130272 got it. I updated the PR to run a read-only README consistency check on PRs targeting main, with actionable mismatch details and a repair command. |
|
@sbates130272 everything fixed in |
|
@2imi9 Stephen may be away for a bit. I can help run CI and review things if need be. Just give me a ping on GitHub. Work on whatever you are most interested in! |
|
@john00003 Thanks! I just finished checking #130. It’s ready for review. |
…sync Upstream ROCm#138 landed the same runner/workflow script split under .github/scripts/, so drop this branch's competing top-level scripts/ move and keep only the README version check. The sync script and its tests move to .github/scripts/workflows/ to match.
| pull_request: | ||
| branches: [main] | ||
| paths: [docker/Dockerfile] | ||
| types: [opened, synchronize, reopened, edited] |
There was a problem hiding this comment.
| types: [opened, synchronize, reopened, edited] | |
| types: [opened, synchronize, reopened] |
I believe edited means this would trigger on things such as modifying a PR's title or description as well. Is my understanding correct?
There was a problem hiding this comment.
Yes, that’s correct. I removed it.
| # stack declared in docker/Dockerfile. | ||
|
|
||
| name: AIC Update README Shields | ||
| name: AIC README Version Check |
There was a problem hiding this comment.
Could we add a paths: filter, that only runs this on changes to docker/Dockerfile, README.md, patches/**, or the script change?
| from urllib.parse import quote | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[2] |
There was a problem hiding this comment.
Should this be parents[3]? My agent flagged that this was failing and proposed a fix:
# test_sync_readme_versions.py:28 — where the fixture dir is made
- (self.root / "scripts" / "workflows").mkdir(parents=True)
+ (self.root / ".github" / "scripts" / "workflows").mkdir(parents=True)
# test_sync_readme_versions.py:40 — where setUp copies to, and run_sync executes from
- return self.root / "scripts" / "workflows" / SCRIPT_NAME
+ return self.root / ".github" / "scripts" / "workflows" / SCRIPT_NAME
mkdir(parents=True) already creates the intermediate .github/, so no extra call is needed. Everything else keys off the script property — setUp's shutil.copy2 destination and run_sync's argv — so those two edits move the whole fixture.
The other fixture paths stay put: dockerfile is self.root / "docker" / "Dockerfile" and readme is self.root / "README.md", which are correct relative to the tmpdir-as-repo-root either way.
Full change
Four edits, verified together above (CHECK_EXIT=0, Ran 12 tests ... OK):
1. parents[2] → parents[3] in both files.
2. The two fixture-layout lines above.
3. The two repair hints in sync-readme-versions.py:243 and :267 — they currently print python3 scripts/workflows/..., a path that doesn't exist.
4. The two test assertions at :132 and :272 that pin those hint strings.
Let me know what your thoughts are!
There was a problem hiding this comment.
Thank you !
parents[2] resolved to .github, so the script was incorrectly looking for .github/docker/Dockerfile. I changed both files to use parents[3], moved the test fixture script under .github/scripts/workflows, and corrected the repair-command paths and assertions.
I verified the fix with the real metadata check and all 12 tests passing. Fixed in 18e77b2.
|
Let me install the updates workflows on my runner and try a CI run! |
3 participants
Motivation
Restore the README version-sync workflow after the source-build stack changed the Dockerfile ARG names.
Technical Details
Read the current component ARGs, update the current badges and stack table, and fail if required metadata or README fields are missing.
Test Plan
Check the current Dockerfile, a synthetic version bump, and missing metadata; run YAML parsing, ShellCheck, and
git diff --check.Test Result
All checks pass locally. The current README stays unchanged, and the version-bump fixture updates every owned field.
Submission Checklist