Skip to content

Commit 54dccc1

Browse files
fix(ci): address code review findings for PR #1884
- Remove push: triggers from reusable workflows (config-checks, golang-checks, image-builds, release) to prevent double-execution when called from ci.yaml - Eliminate ~80 lines of duplicated variable calculation logic in image-builds.yaml and release.yaml - Make yq a hard requirement for YAML merging instead of falling back to cat concatenation which produces invalid YAML - Replace echo -e with printf '%b' in env-to-values.sh for portability - Add use_values_override input to e2e-tests workflow_dispatch - Fix shebang consistency in get-latest-images.sh (#!/usr/bin/env bash) - Document 8-char SHA truncation assumption in get-latest-images.sh - Remove non-deterministic date from generated values file headers - Remove trailing whitespace in code-scanning.yaml
1 parent 4f8ddc8 commit 54dccc1

File tree

10 files changed

+32
-82
lines changed

10 files changed

+32
-82
lines changed

.github/scripts/generate-values-overrides.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ MIG_MANAGER_IMAGE="$4"
4141
# Generate values override file
4242
cat > "${OUTPUT_FILE}" <<EOF
4343
# Generated by generate-values-overrides.sh
44-
# Date: $(date -u +"%Y-%m-%d %H:%M:%S UTC")
4544
#
4645
# This file overrides default GPU Operator component images with
4746
# specific versions for forward compatibility testing.

.github/scripts/get-latest-images.sh

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#!/bin/bash
1+
#!/usr/bin/env bash
22
# Copyright NVIDIA CORPORATION
33
#
44
# Licensed under the Apache License, Version 2.0 (the "License");
@@ -51,7 +51,9 @@ esac
5151

5252
echo "Fetching latest commit from ${GITHUB_REPO}..." >&2
5353

54-
# Get the latest commit SHA from the main branch using GitHub API
54+
# Get the latest commit SHA from the main branch using GitHub API.
55+
# NOTE: We use 8-char truncated SHAs as image tags. This must match the
56+
# tag convention used by each component's CI pipeline when publishing images.
5557
GITHUB_API_URL="https://api.github.com/repos/${GITHUB_REPO}/commits/main"
5658

5759
# Use GITHUB_TOKEN if available for authentication (higher rate limits)

.github/workflows/code-scanning.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ jobs:
4545
- shell: bash
4646
run: |
4747
make build
48-
48+
4949
- name: Perform CodeQL Analysis
5050
uses: github/codeql-action/analyze@v4
5151
with:

.github/workflows/config-checks.yaml

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,6 @@
1515
name: Configuration Checks
1616

1717
on:
18-
push:
19-
branches:
20-
- "pull-request/[0-9]+"
21-
- main
22-
- release-*
2318
workflow_call:
2419
workflow_dispatch:
2520

.github/workflows/e2e-tests.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ on:
4949
description: 'Operator version to test (override)'
5050
required: false
5151
type: string
52+
use_values_override:
53+
description: 'Use values-overrides artifact for component image configuration'
54+
required: false
55+
type: boolean
56+
default: false
5257

5358
jobs:
5459
variables:

.github/workflows/golang-checks.yaml

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,6 @@
1515
name: Golang Checks
1616

1717
on:
18-
push:
19-
branches:
20-
- "pull-request/[0-9]+"
21-
- main
22-
- release-*
2318
workflow_call:
2419
workflow_dispatch:
2520

.github/workflows/image-builds.yaml

Lines changed: 7 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,6 @@
1515
name: Image Builds
1616

1717
on:
18-
push:
19-
branches:
20-
- "pull-request/[0-9]+"
21-
- main
22-
- release-*
2318
workflow_call:
2419
inputs:
2520
commit_short_sha:
@@ -34,7 +29,6 @@ on:
3429
operator_image_base:
3530
required: true
3631
type: string
37-
workflow_dispatch:
3832

3933
jobs:
4034
variables:
@@ -48,47 +42,19 @@ jobs:
4842
operator_image_amd64: ${{ steps.vars.outputs.operator_image_amd64 }}
4943
operator_image_multiarch: ${{ steps.vars.outputs.operator_image_multiarch }}
5044
steps:
51-
- name: Checkout code
52-
if: ${{ github.event_name != 'workflow_call' }}
53-
uses: actions/checkout@v6
5445
- name: Calculate build variables
5546
id: vars
5647
run: |
57-
# Use inputs from workflow_call if available, otherwise calculate
58-
if [[ "${{ github.event_name }}" == "workflow_call" ]]; then
59-
COMMIT_SHORT_SHA="${{ inputs.commit_short_sha }}"
60-
LABEL_IMAGE_SOURCE="${{ inputs.label_image_source }}"
61-
PUSH_ON_BUILD="${{ inputs.push_on_build }}"
62-
OPERATOR_IMAGE_BASE="${{ inputs.operator_image_base }}"
63-
else
64-
# Calculate for standalone runs
65-
COMMIT_SHORT_SHA="${GITHUB_SHA:0:8}"
66-
67-
REPO_FULL_NAME="${{ github.event.pull_request.head.repo.full_name }}"
68-
if [[ -z "${REPO_FULL_NAME}" ]]; then
69-
REPO_FULL_NAME="${{ github.repository }}"
70-
fi
71-
LABEL_IMAGE_SOURCE="https://github.com/${REPO_FULL_NAME}"
72-
73-
PUSH_ON_BUILD="false"
74-
if [[ "${{ github.actor }}" != "dependabot[bot]" ]]; then
75-
if [[ "${{ github.event_name }}" == "pull_request" && "${{ github.event.pull_request.head.repo.full_name }}" == "${{ github.repository }}" ]]; then
76-
PUSH_ON_BUILD="true"
77-
elif [[ "${{ github.event_name }}" == "push" ]]; then
78-
PUSH_ON_BUILD="true"
79-
elif [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
80-
PUSH_ON_BUILD="true"
81-
fi
82-
fi
83-
84-
OPERATOR_IMAGE_BASE="ghcr.io/nvidia/gpu-operator"
85-
fi
86-
48+
COMMIT_SHORT_SHA="${{ inputs.commit_short_sha }}"
49+
LABEL_IMAGE_SOURCE="${{ inputs.label_image_source }}"
50+
PUSH_ON_BUILD="${{ inputs.push_on_build }}"
51+
OPERATOR_IMAGE_BASE="${{ inputs.operator_image_base }}"
52+
8753
# Calculate derived image names
8854
OPERATOR_IMAGE_ARM64="${OPERATOR_IMAGE_BASE}:${COMMIT_SHORT_SHA}-arm64"
8955
OPERATOR_IMAGE_AMD64="${OPERATOR_IMAGE_BASE}:${COMMIT_SHORT_SHA}-amd64"
9056
OPERATOR_IMAGE_MULTIARCH="${OPERATOR_IMAGE_BASE}:${COMMIT_SHORT_SHA}"
91-
57+
9258
# Output all variables
9359
echo "commit_short_sha=${COMMIT_SHORT_SHA}" >> $GITHUB_OUTPUT
9460
echo "label_image_source=${LABEL_IMAGE_SOURCE}" >> $GITHUB_OUTPUT
@@ -97,7 +63,7 @@ jobs:
9763
echo "operator_image_arm64=${OPERATOR_IMAGE_ARM64}" >> $GITHUB_OUTPUT
9864
echo "operator_image_amd64=${OPERATOR_IMAGE_AMD64}" >> $GITHUB_OUTPUT
9965
echo "operator_image_multiarch=${OPERATOR_IMAGE_MULTIARCH}" >> $GITHUB_OUTPUT
100-
66+
10167
# Display for debugging
10268
echo "::notice::Commit SHA: ${COMMIT_SHORT_SHA}"
10369
echo "::notice::Push on build: ${PUSH_ON_BUILD}"

.github/workflows/release.yaml

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@
1515
name: Release
1616

1717
on:
18-
push:
19-
branches:
20-
- main
2118
workflow_call:
2219
inputs:
2320
commit_short_sha:
@@ -37,34 +34,24 @@ jobs:
3734
operator_image_latest: ${{ steps.vars.outputs.operator_image_latest }}
3835
bundle_image: ${{ steps.vars.outputs.bundle_image }}
3936
steps:
40-
- name: Checkout code
41-
if: ${{ github.event_name != 'workflow_call' }}
42-
uses: actions/checkout@v6
4337
- name: Calculate release variables
4438
id: vars
4539
run: |
46-
# Use inputs from workflow_call if available
47-
if [[ "${{ github.event_name }}" == "workflow_call" ]]; then
48-
COMMIT_SHORT_SHA="${{ inputs.commit_short_sha }}"
49-
OPERATOR_IMAGE_BASE="${{ inputs.operator_image_base }}"
50-
else
51-
# Calculate for standalone runs
52-
COMMIT_SHORT_SHA="${GITHUB_SHA:0:8}"
53-
OPERATOR_IMAGE_BASE="ghcr.io/nvidia/gpu-operator"
54-
fi
55-
40+
COMMIT_SHORT_SHA="${{ inputs.commit_short_sha }}"
41+
OPERATOR_IMAGE_BASE="${{ inputs.operator_image_base }}"
42+
5643
# Calculate derived values
5744
OPERATOR_IMAGE_SOURCE="${OPERATOR_IMAGE_BASE}:${COMMIT_SHORT_SHA}"
5845
OPERATOR_IMAGE_LATEST="${OPERATOR_IMAGE_BASE}:main-latest"
5946
BUNDLE_IMAGE="ghcr.io/nvidia/gpu-operator/gpu-operator-bundle:main-latest"
60-
47+
6148
# Output all variables
6249
echo "commit_short_sha=${COMMIT_SHORT_SHA}" >> $GITHUB_OUTPUT
6350
echo "operator_image_base=${OPERATOR_IMAGE_BASE}" >> $GITHUB_OUTPUT
6451
echo "operator_image_source=${OPERATOR_IMAGE_SOURCE}" >> $GITHUB_OUTPUT
6552
echo "operator_image_latest=${OPERATOR_IMAGE_LATEST}" >> $GITHUB_OUTPUT
6653
echo "bundle_image=${BUNDLE_IMAGE}" >> $GITHUB_OUTPUT
67-
54+
6855
# Display for debugging
6956
echo "::notice::Releasing: ${OPERATOR_IMAGE_SOURCE} → ${OPERATOR_IMAGE_LATEST}"
7057

tests/scripts/env-to-values.sh

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ OUTPUT_FILE="$1"
4242
# Start with header
4343
cat > "${OUTPUT_FILE}" <<EOF
4444
# Generated from environment variables by env-to-values.sh
45-
# Date: $(date -u +"%Y-%m-%d %H:%M:%S UTC")
4645
#
4746
# This file contains GPU Operator configuration derived from test
4847
# environment variables.
@@ -80,13 +79,15 @@ fi
8079
# Write operator configuration if any
8180
if [[ -n "${OPERATOR_CONFIG}" ]]; then
8281
echo "operator:" >> "${OUTPUT_FILE}"
83-
echo -e "${OPERATOR_CONFIG}" >> "${OUTPUT_FILE}"
82+
printf '%b' "${OPERATOR_CONFIG}" >> "${OUTPUT_FILE}"
83+
echo "" >> "${OUTPUT_FILE}"
8484
fi
8585

8686
# Write validator configuration if any
8787
if [[ -n "${VALIDATOR_CONFIG}" ]]; then
8888
echo "validator:" >> "${OUTPUT_FILE}"
89-
echo -e "${VALIDATOR_CONFIG}" >> "${OUTPUT_FILE}"
89+
printf '%b' "${VALIDATOR_CONFIG}" >> "${OUTPUT_FILE}"
90+
echo "" >> "${OUTPUT_FILE}"
9091
fi
9192

9293
# Container Toolkit configuration

tests/scripts/install-operator.sh

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,13 @@ if [[ "${USE_VALUES_FILE}" == "true" ]]; then
4848

4949
# Create a combined values file using yq for proper YAML merging
5050
COMBINED_VALUES=$(mktemp)
51-
if command -v yq >/dev/null 2>&1; then
52-
# yq merges YAML properly, with later files taking precedence
53-
yq ea '. as $item ireduce ({}; . * $item )' "${VALUES_FILE}" "${TEMP_ENV_VALUES}" > "${COMBINED_VALUES}"
54-
else
55-
echo "Warning: yq not found, falling back to concatenation (may have issues with duplicate keys)" >&2
56-
cat "${VALUES_FILE}" "${TEMP_ENV_VALUES}" > "${COMBINED_VALUES}"
51+
if ! command -v yq >/dev/null 2>&1; then
52+
echo "Error: yq is required to merge YAML values files but was not found in PATH." >&2
53+
echo "Install yq: https://github.com/mikefarah/yq" >&2
54+
exit 1
5755
fi
56+
# yq merges YAML properly, with later files taking precedence
57+
yq ea '. as $item ireduce ({}; . * $item )' "${VALUES_FILE}" "${TEMP_ENV_VALUES}" > "${COMBINED_VALUES}"
5858
VALUES_FILE="${COMBINED_VALUES}"
5959
else
6060
VALUES_FILE="${TEMP_ENV_VALUES}"

0 commit comments

Comments
 (0)