Skip to content

Conversation

@wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

We are writing a default read-only mirror with

spack mirror add --scope site --unsigned ghcr-${SPACK_VERSION} oci://ghcr.io/eic/spack-${SPACK_VERSION}

but when we pass SPACK_VERSION as a sha that's not the right location.

This PR ensures that the *_VERSION and *_SHA build arguments are both accepted, and makes it clearer which is which.

What kind of change does this PR introduce?

  • Bug fix (issue: incorrect default read-only mirror)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Copilot AI review requested due to automatic review settings November 21, 2025 22:43
Copilot finished reviewing on behalf of wdconinc November 21, 2025 22:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses a critical bug in the mirror path construction by distinguishing between version identifiers (branch/tag names) and commit SHA values. When SPACK_VERSION or similar variables were passed as commit SHAs, the buildcache mirror URLs would be incorrect (e.g., oci://ghcr.io/eic/spack-<sha> instead of oci://ghcr.io/eic/spack-<version>).

Key Changes:

  • Introduced separate *_SHA build arguments alongside existing *_VERSION arguments across all Dockerfiles and CI configurations
  • Modified git checkout commands to use ${*_SHA:-${*_VERSION}} fallback pattern, preferring SHA when available
  • Updated CI pipelines to resolve git references and pass both VERSION and SHA as separate build arguments

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
containers/debian/Dockerfile Added *_SHA ARG declarations for SPACK, SPACKPACKAGES, KEY4HEPSPACK, and EICSPACK; updated git checkout commands to use SHA with VERSION fallback
containers/eic/Dockerfile Renamed *_VERSION to *_SHA for EDM4EIC, EICRECON, EPIC, JUGGLER, BENCHMARK, and CAMPAIGNS; updated git checkout commands with default fallbacks
.gitlab-ci.yml Added *_SHA build arguments alongside *_VERSION; updated to resolve git refs separately for both values
.github/workflows/build-push.yml Added SHA output variables alongside VERSION; updated build-args to pass both SHA and VERSION values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wdconinc and others added 2 commits November 21, 2025 16:49
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wdconinc wdconinc requested a review from Copilot November 22, 2025 01:53
Copilot finished reviewing on behalf of wdconinc November 22, 2025 01:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wdconinc wdconinc requested a review from veprbl November 22, 2025 04:49
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@wdconinc wdconinc enabled auto-merge (squash) November 23, 2025 15:58
@wdconinc wdconinc disabled auto-merge November 23, 2025 23:00
@wdconinc wdconinc merged commit 484db61 into master Nov 23, 2025
21 of 22 checks passed
wdconinc added a commit that referenced this pull request Nov 27, 2025
* fix: be more explicit about *_VERSION (tag, branch) and *_SHA

* fix: default *_SHA to empty

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: check if *_VERSION is defined before setting *_SHA

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants