docs: correct pipeline, image and version-update documentation - #422
Merged
Merged
Conversation
The documentation had drifted from the code in several places that would send a reader, human or agent, down the wrong path. AGENTS.md: - the workflow tree listed three of the six workflows, omitting the two reusable workflows that do all the building and the backport workflow - the image list named three images; cuda_devel and cuda_runtime were missing, as was any mention of the environments built only on GitLab CI - the spack-environment tree showed two of the nine environments - "Update versions in both places" named xl and cuda only. There are nine environments with an epic overlay, and the thing that actually matters is the trailing marker comment the Dockerfile keys its sed overrides on, which was not documented at all - "Tags are automatically cleaned on PR close by cleanup workflows" is not true for GitHub Actions: no such workflow exists, which is what issue #89 is about and PR #90 proposes. GitLab does clean up, via clean_unstable_mr and clean_pipeline. Say which is which .github/copilot-instructions.md carried the same "both xl and cuda" claim; corrected in step with AGENTS.md. The rest of that file correctly defers to AGENTS.md as the primary reference and is left alone. docs/build-pipeline.md described the pipeline as it was before the reusable workflows were split out: build-push.yml appeared to run the builds itself, and the diagrams showed two architectures where three are built. Add a table of the two reusable workflows and what each produces, and correct the architecture set to amd64, amd64_v3 and arm64. README.md linked to the GitLab mirror for files that live in this repository, and the link captioned "spack.sh" actually pointed at packages.yaml while the text describes editing the cherry-pick list, which is in spack-packages.sh. Point both at the files here. The historical merge-request link is left as-is, since it is a reference to something that only exists on GitLab. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B6394UjZvzHPVygCvjRc9n
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated pipeline documentation still has a couple of concrete accuracy gaps (notably around amd64_v3 coverage in the EIC portion and base-manifest wording) that should be corrected before merging.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates repository documentation to match the current CI/workflow structure, image set, and version-update practices for the EIC container build system, reducing the risk of contributors following outdated guidance.
Changes:
- Correct workflow inventory and clarify that
build-push.ymlorchestrates builds via the reusablebuild-base.yml/build-eic.ymlworkflows. - Update version-update guidance to reflect all nine environments and document the load-bearing version marker comments used by
containers/eic/Dockerfile. - Fix README links to point to the correct in-repo versioning/cherry-pick files, and update pipeline documentation/diagrams for current architecture coverage.
File summaries
| File | Description |
|---|---|
| README.md | Fixes incorrect/misleading links and clarifies where cherry-picks vs. package pins are updated. |
| docs/build-pipeline.md | Updates pipeline explanation/diagrams to reflect reusable workflows and current architecture set. |
| AGENTS.md | Refreshes repository “source of truth” docs for workflows, image set, and version-tracking rules (incl. marker comments). |
| .github/copilot-instructions.md | Aligns agent guidance with the nine-environment reality and marker-comment requirement. |
Review details
Suppressed comments (1)
docs/build-pipeline.md:105
- The base-manifest job runs for
debian_stable_base,cuda_devel, andcuda_runtime, but onlydebian_stable_baseis built foramd64_v3/arm64today. Describing the manifest creation as always combiningamd64 + amd64_v3 + arm64is not accurate for the CUDA base images (they currently have onlyamd64).
GH->>R: Create manifest list<br/>combining amd64 + amd64_v3 + arm64
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wdconinc
enabled auto-merge (squash)
September 14, 2026 17:39
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
auto-merge was automatically disabled
September 14, 2026 17:43
Head branch was pushed to by a user without write access
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A couple of newly added pipeline-doc statements are slightly inaccurate/underspecified (reusable workflow inputs and manifest-arch wording) and should be corrected to avoid misleading readers.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
README.md:11
- Minor wording/capitalization: this heading currently reads a bit ungrammatical (“on spack package repository”). Using “in the Spack packages repository” (and capitalizing Spack) makes the sentence clearer.
docs/build-pipeline.md:12 - The reusable-workflows input list is missing
job-name-prefix, which is a required input forbuild-eic.yml(workflow_call.inputs.job-name-prefix). Mentioning it here will prevent readers from assumingmatrix/jobsare sufficient for all calls.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Capybara summary for PR 422Last updated 2026-09-15T01:02-04:00 3481b13 |
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
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.
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
Documentation that had drifted from the code, in places that would send a reader — human or agent — down the wrong path.
AGENTS.mdbuild-push.yml,docs.yml,mirror.yamlbuild-base.yml,build-eic.yml) andpr-backport.ymlwere missingcuda_develandcuda_runtimemissing, as was any mention of the seven environments built only on GitLab CIspack-environment/showsciandxlxl/epic,cuda/epic"clean_unstable_mrandclean_pipelineThe version-update section also never mentioned the thing that actually matters: the trailing
# EDM4EIC_VERSION/# EICRECON_VERSION/# EPIC_VERSIONmarker comments are load-bearing —containers/eic/Dockerfilekeys itssedoverrides on them, and reWording one silently breaks the override. Now documented. (#420 makes that failure loud rather than silent.).github/copilot-instructions.mdCarried the same "both
xlandcuda" claim; corrected in step. The rest of that file correctly defers toAGENTS.mdas the primary reference rather than duplicating it, so it is otherwise left alone.docs/build-pipeline.mdDescribed the pipeline as it was before the reusable workflows were split out —
build-push.ymlappeared to run the builds itself, and the diagrams showed two architectures where three are built. Adds a table of the two reusable workflows and what each produces, and corrects the architecture set toamd64,amd64_v3,arm64.README.mdLinked to the GitLab mirror for files that live in this repository. Worse, the link captioned
spack.shactually pointed atpackages.yaml, while the surrounding text describes editing the cherry-pick list — which is inspack-packages.sh, a third file. All three now point at the right files here. The historical merge-request link is left as-is, since it references something that only exists on GitLab.Verification
Documentation only — no build or runtime behaviour changes. Every claim above was checked against the tree (workflow files present, environment count, marker locations, which CI has cleanup jobs). Mermaid fences verified balanced;
pre-commitpasses apart from the pre-existing hadolint parse failure that #416 addresses.What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply
AI usage: the drift was found by checking each documented claim against the repository with Claude Code, and this PR was prepared with it. Worth a maintainer's eye on two judgement calls: the list of GitLab-only images, and whether the buildcache-cleanup wording should reference #89/#90 at all or just state the GitHub situation.
🤖 Generated with Claude Code
https://claude.ai/code/session_01B6394UjZvzHPVygCvjRc9n
Generated by Claude Code