Skip to content

feat :Testing workflow to push images#405

Open
arekay-nv wants to merge 4 commits into
mainfrom
arekay/push_image_workflow
Open

feat :Testing workflow to push images#405
arekay-nv wants to merge 4 commits into
mainfrom
arekay/push_image_workflow

Conversation

@arekay-nv

Copy link
Copy Markdown
Collaborator

What does this PR do?

Adds a reproducible way to build and publish the endpoints client image to
ghcr.io/mlcommons/endpoints:

  • scripts/push_docker_image.sh — builds scripts/Dockerfile.dev at a chosen
    git ref via docker buildx and pushes it, tagged with both the short commit SHA
    and the ref name so consumers can pin either.
  • .github/workflows/publish-image.yml — a manually-triggered
    (workflow_dispatch) workflow that runs the script, so a publish can be kicked
    off from the Actions tab against any selected branch/tag/SHA without a local
    Docker setup.

The workflow reuses the script (single source of truth for the tag scheme) rather
than duplicating build logic.

Behavior & design

Aspect Choice
Default platform linux/amd64 (what benchmark clients run on); multi-arch is one flag/input away
Multi-arch --multi-arch / linux/amd64,linux/arm64 publishes a manifest list usable on x86 and arm
Tags :<short-sha> + :<sanitized-ref>
Builder self-provisions a docker-container buildx builder (the default docker driver can neither --push nor build multi-platform)
Auth script logs in via GHCR_USER/GHCR_TOKEN if set, else assumes docker login; workflow uses github.actor + the automatic GITHUB_TOKEN
Safety refuses to publish a :$SHA tag from a dirty tree (--allow-dirty to override); optional ref checkout is restored on exit
PROVISION_DSR1 passthrough build-arg (default = Dockerfile default 1); set 0 for a lean client-only image

CI prerequisites (one-time, outside this PR)

  • Workflow must land on the default branch before the "Run workflow" button appears.
  • permissions: packages: write is set in the workflow; the org must allow the
    GITHUB_TOKEN to write packages, and the endpoints package must grant this repo
    Write access (Package → Manage Actions access). Usually automatic since the
    package lives under this repo's org.

Note: GITHUB_TOKEN authenticates as github-actions[bot] (scoped to this repo);
username: github.actor only sets the login name. For a push attributed to a
specific account, supply that account's PAT as a secret instead.

Validation

  • Multi-arch dry build (--multi-arch --no-push): both linux/amd64 (emulated)
    and linux/arm64 (native) compiled all stages, exit 0.
  • Script is clean under macOS bash 3.2.57 and bash -n; the empty-array
    expansion and tag sanitizer were unit-checked; the dirty-tree guard was exercised
    across push/dry/allow-dirty/clean/checked-out cases.
  • Reviewed via a two-model council (Codex + Claude); all findings fixed: detached-HEAD
    ref resolution, Docker-tag sanitization beyond /, bash-3.2 empty-array crash, and
    the dirty-tree publish guard. Workflow security posture confirmed (inputs passed via
    env: not inline ${{ }}, actions pinned to SHAs, minimal contents:read +
    packages:write).

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor/cleanup

Related issues

Testing

  • Tests added/updated
  • All tests pass locally
  • Manual testing completed

Checklist

  • Code follows project style
  • Pre-commit hooks pass
  • Documentation updated (if needed)

Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
@arekay-nv
arekay-nv requested a review from a team July 9, 2026 16:20
@github-actions
github-actions Bot requested a review from nvzhihanj July 9, 2026 16:20
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new bash script, scripts/push_docker_image.sh, designed to build and push the endpoints client Docker image to a registry with support for multi-platform builds, custom git refs, and registry authentication. The review feedback highlights two important shell scripting improvements: using printf instead of echo to safely pipe the registry login token, and properly quoting the BUILD_ARGS array expansion to prevent word splitting.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

LOGIN_TOKEN="${GHCR_TOKEN:-${GITHUB_TOKEN:-}}"
if [[ "$PUSH" == "1" && -n "$LOGIN_TOKEN" ]]; then
echo ">> Logging in to ${REGISTRY_HOST} as ${LOGIN_USER:-<token>}"
echo "$LOGIN_TOKEN" | docker login "$REGISTRY_HOST" -u "${LOGIN_USER:-x-access-token}" --password-stdin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using echo to pipe sensitive tokens or arbitrary strings can lead to unexpected behavior if the token starts with a hyphen (e.g., -n or -e), as echo may interpret it as an option. It is safer and more robust to use printf '%s\n' instead.

Suggested change
echo "$LOGIN_TOKEN" | docker login "$REGISTRY_HOST" -u "${LOGIN_USER:-x-access-token}" --password-stdin
printf '%s\n' "$LOGIN_TOKEN" | docker login "$REGISTRY_HOST" -u "${LOGIN_USER:-x-access-token}" --password-stdin

-f "$DOCKERFILE" \
-t "${IMAGE}:${SHORT_SHA}" \
-t "${IMAGE}:${REF_TAG}" \
${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"} \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The array expansion ${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"} is missing outer double quotes. Without outer double quotes, any array elements containing spaces or glob characters will undergo word splitting and pathname expansion when expanded. To safely preserve spaces within array elements while maintaining compatibility with set -u in older Bash versions, wrap the entire expansion in double quotes.

Suggested change
${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"} \
"${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" \

@arekay-nv arekay-nv left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review Council — Multi-AI Code Review

Reviewed by Codex (gpt-5.5, xhigh) + Claude. 3 inline findings below (2 medium, 1 low; no critical/high).


# Building the working tree as-is (no distinct ref checked out): don't publish a
# :$SHORT_SHA tag whose '.' build context doesn't match commit $SHORT_SHA.
if [[ -z "${DID_CHECKOUT:-}" && "$ALLOW_DIRTY" != "1" ]] && tree_has_tracked_changes; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Codex + Claude] medium (build reproducibility + context): The dirty guard here uses git diff --quiet HEAD, which ignores untracked files. But scripts/Dockerfile.dev:49 does COPY src/ ./src/, so an untracked file under src/ gets baked into the image — a local --push can publish :${SHORT_SHA} whose contents don't match commit ${SHORT_SHA}.

Separately, the comment at line 124 claims ".dockerignore governs the build context", but no .dockerignore exists in the repo, so the whole working tree is uploaded as build context (docker buildx build … ., line 197), pulling in large untracked dirs (cached_datasets/, gtc_results/, dsr1_v61_seed_logs_*/) on every local run.

Fix: add a .dockerignore (exclude artifacts + anything not COPYed); for the SHA-tag correctness, also include untracked files under COPYed paths in the cleanliness check, or build from git archive.

# Check out the ref *name* (not github.sha) so a local branch head exists
# and the script can resolve/tag it; blank input falls back to the
# dispatched branch/tag. Full history for `git rev-parse --short`.
ref: ${{ inputs.ref || github.ref_name }}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Codex] medium (correctness): This checks out the dispatch ref directly, replacing the tree before the run step. Building a ref that predates this workflow means ./scripts/push_docker_image.sh won't exist and the run fails — even though the UI advertises building arbitrary refs. It also lets the target ref supply the script that runs after GHCR login.

Fix: keep the checkout on the workflow's own (trusted) ref, and pass the target via ENDPOINTS_REF so the script switches refs only for the Docker build. (Ensure the target ref is fetched — the script's origin/<ref> fallback needs the object present.)

- name: Set up QEMU
uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0

- name: Set up Docker Buildx

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Claude] low (design): This provisions a buildx builder that the script never uses — push_docker_image.sh always creates and selects its own endpoints-buildx (docker-container) builder (lines 173/191). buildx is preinstalled on ubuntu-latest, so this step is effectively idle. Either drop it, or have the script honor a caller-supplied builder. (The Set up QEMU step above is needed for multi-arch — keep it.)

@arekay-nv

Copy link
Copy Markdown
Collaborator Author

Review Council — Multi-AI Code Review

Reviewed by: Codex (gpt-5.5, xhigh) + Claude | Depth: standard (low-severity C included on request)

Found 3 issues, all posted inline. No critical/high.

# File Line Severity Category Reviewer(s) Summary
A scripts/push_docker_image.sh 158 medium reproducibility Both Dirty guard uses git diff --quiet HEAD → ignores untracked files; Dockerfile.dev COPY src/ can bake them into :{sha}. Also no .dockerignore exists (contradicts the line-124 comment) → whole tree uploaded as context.
B .github/workflows/publish-image.yml 56 medium bug Codex Checks out arbitrary dispatch ref directly; refs predating this workflow lack the script and fail, and the target ref supplies the post-login script.
C .github/workflows/publish-image.yml 64 low design Claude Set up Docker Buildx provisions a builder the script never uses (it always creates its own endpoints-buildx). QEMU step is needed — keep.

ℹ️ The existing gemini-code-assist comment on push_docker_image.sh:196 (array expansion "missing outer quotes") is a false positive${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"} is already the correct Bash 3.2 / set -u empty-array guard; the suggested rewrite is not an improvement.

All comments are neutral (event: COMMENT) — no approve/request-changes.

Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@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.

1 participant