Skip to content

fix(ci): add GHA structured output to verify-ci #9049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devguyio wants to merge 2 commits into
base: main
Choose a base branch
from
+112 −9
Open

fix(ci): add GHA structured output to verify-ci #9049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08c7bad
fix(ci): add GHA structured output to verify-ci and test-shard
devguyio Jul 21, 2026
0e2842a
DO NOT MERGE: break unit test + bad formatting to test GHA output
devguyio Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/verify.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Verify
name: Source Freshness

on:
pull_request:
Expand All @@ -7,7 +7,8 @@ on:
- release-4.22

jobs:
verify:
call-main-verify:
name: Call main verify workflow
uses: openshift/hypershift/.github/workflows/verify-reusable.yaml@main
permissions:
contents: read
112 changes: 107 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
SHELL := /bin/bash
DIR := ${CURDIR}

# Image URL to use all building/pushing image targets
Expand Down Expand Up @@ -34,6 +35,7 @@ CODESPELL := $(PYTHON_VENV)/bin/$(CODESPELL_BIN)
GITLINT := $(PYTHON_VENV)/bin/$(GITLINT_BIN)

PROMTOOL=$(abspath $(TOOLS_BIN_DIR)/promtool)
GOTESTSUM := $(abspath $(TOOLS_BIN_DIR)/gotestsum)

# Setup envtest for running tests that require a Kubernetes API server
# SETUP_ENVTEST_VER is the version of setup-envtest to use, matching the version in hack/tools/go.mod
Expand Down Expand Up @@ -158,9 +160,94 @@ verify-crd-schema: $(CRD_SCHEMA_CHECK) ## Verify CRD schemas for breaking change
.PHONY: verify-parallel
verify-parallel: verify-codespell verify-codecov verify-api-deps verify-crd-schema lint cpo-container-sync run-gitlint verify-docs-nav

FAILURES_FILE := $(shell mktemp)
SNAPSHOT_FILE := $(shell mktemp)
Comment on lines +163 to +164

@coderabbitai coderabbitai Bot Jul 22, 2026

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Temp files leak on every invocation, amplified by recursive $(MAKE) calls.

FAILURES_FILE/SNAPSHOT_FILE are created via $(shell mktemp) in a := assignment, which runs at Makefile-parse time for every make invocation, even unrelated targets. Worse, run-step invokes $(MAKE) $(1) (lines 231-238, 501-502), and each such sub-make re-parses the whole Makefile from scratch, re-running mktemp again — so a single make verify-ci run leaks roughly 2×(N+1) temp files (8 steps → ~18 files), and SNAPSHOT_FILE is never removed anywhere (check-failures, lines 216-226, only cleans FAILURES_FILE).

🛠️ Proposed fix: export so sub-makes reuse the parent's files, and clean up both 
-FAILURES_FILE := $(shell mktemp)
-SNAPSHOT_FILE := $(shell mktemp)
+FAILURES_FILE ?= $(shell mktemp)
+SNAPSHOT_FILE ?= $(shell mktemp)
+export FAILURES_FILE SNAPSHOT_FILE
 define check-failures
 	`@if` [ -s $(FAILURES_FILE) ]; then \
 		echo ""; \
 		echo "Failed steps:"; \
 		cat $(FAILURES_FILE); \
 		rm -f $(FAILURES_FILE); \
+		rm -f $(SNAPSHOT_FILE); \
 		exit 1; \
 	else \
 		rm -f $(FAILURES_FILE); \
+		rm -f $(SNAPSHOT_FILE); \
 	fi
 endef
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 162 - 163, Change the top-level FAILURES_FILE and
SNAPSHOT_FILE definitions to be exported so recursive invocations through
run-step reuse the parent’s temporary files instead of creating new ones during
Makefile parsing. Update the cleanup logic in check-failures to remove both
files, including the snapshot file, while preserving existing failure handling.


ifdef GITHUB_ACTIONS
define run-step
@git diff --name-only HEAD 2>/dev/null | while read f; do echo "$$f $$(git hash-object "$$f" 2>/dev/null)"; done > $(SNAPSHOT_FILE); \
git ls-files --others --exclude-standard 2>/dev/null | while read f; do echo "$$f $$(git hash-object "$$f" 2>/dev/null)"; done >> $(SNAPSHOT_FILE); \
echo "::group::$(1)"; \
$(MAKE) $(1) 2>&1; rc=$$?; \
new_dirty=""; \
for f in $$(git diff --name-only HEAD 2>/dev/null) $$(git ls-files --others --exclude-standard 2>/dev/null); do \
cur_hash=$$(git hash-object "$$f" 2>/dev/null); \
prev_hash=$$(grep "^$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \
if [ "$$cur_hash" != "$$prev_hash" ]; then \
new_dirty="$$new_dirty $$f"; \
fi; \
done; \
Comment on lines +173 to +179

@coderabbitai coderabbitai Bot Jul 22, 2026

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Dirty-file detection has a few correctness gaps (also present in the mirrored block at lines 197-203).

  • grep "^$$f " treats the filename as a basic regex, so a literal . (present in nearly every Go/YAML filename) matches any character, risking false matches against a different file with a similar name.
  • The for f in $$(git diff ...) $$(git ls-files ...) loops (172, 197) word-split on unquoted command substitution, breaking on filenames containing spaces.
  • If a step deletes a previously-clean tracked file, git hash-object on the now-missing file fails silently (cur_hash=""), and since the file was never in $(SNAPSHOT_FILE) (it wasn't dirty pre-step), prev_hash is also "" — the deletion is never flagged as new_dirty.
🩹 Suggested fix for the regex-matching issue 
-		prev_hash=$$(grep "^$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \
+		prev_hash=$$(grep -F "$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \

The deleted-file case would need a sentinel (e.g. treat a missing file as a distinct hash value rather than empty string) to be fully correct.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for f in $$(git diff --name-only HEAD 2>/dev/null) $$(git ls-files --others --exclude-standard 2>/dev/null); do \
cur_hash=$$(git hash-object "$$f" 2>/dev/null); \
prev_hash=$$(grep "^$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \
if [ "$$cur_hash" != "$$prev_hash" ]; then \
new_dirty="$$new_dirty $$f"; \
fi; \
done; \
for f in $$(git diff --name-only HEAD 2>/dev/null) $$(git ls-files --others --exclude-standard 2>/dev/null); do \
cur_hash=$$(git hash-object "$$f" 2>/dev/null); \
prev_hash=$$(grep -F "$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \
if [ "$$cur_hash" != "$$prev_hash" ]; then \
new_dirty="$$new_dirty $$f"; \
fi; \
done; \
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 172 - 178, Fix dirty-file detection in both mirrored
loops by preserving filenames containing spaces, matching snapshot paths
literally rather than as regular expressions, and assigning a distinct sentinel
when git hash-object cannot hash a missing file so deletions are added to
new_dirty. Update the loops around cur_hash, prev_hash, and new_dirty
consistently in both blocks.

if [ $$rc -ne 0 ]; then \
echo "::error::Step '$(1)' failed"; \
echo "$(1)" >> $(FAILURES_FILE); \
fi; \
if [ -n "$$new_dirty" ]; then \
for f in $$new_dirty; do \
diff_hunks=$$(git diff HEAD -- "$$f" 2>/dev/null | sed -n 's/^@@ .* +\([0-9,]*\) @@.*/\1/p'); \
if [ -n "$$diff_hunks" ]; then \
echo "$$diff_hunks" | while IFS=, read start count; do \
count=$${count:-1}; \
[ "$$count" -eq 0 ] && count=1; \
end=$$((start + count - 1)); \
echo "::error file=$$f,line=$$start,endLine=$$end,title=make $(1)::Not up to date. Run 'make $(1)' locally and commit the result."; \
done; \
else \
echo "::error file=$$f,title=make $(1)::Not up to date. Run 'make $(1)' locally and commit the result."; \
fi; \
done; \
echo "$(1):dirty" >> $(FAILURES_FILE); \
fi; \
echo "::endgroup::"
endef
else
define run-step
@git diff --name-only HEAD 2>/dev/null | while read f; do echo "$$f $$(git hash-object "$$f" 2>/dev/null)"; done > $(SNAPSHOT_FILE); \
git ls-files --others --exclude-standard 2>/dev/null | while read f; do echo "$$f $$(git hash-object "$$f" 2>/dev/null)"; done >> $(SNAPSHOT_FILE); \
$(MAKE) $(1) 2>&1; rc=$$?; \
new_dirty=""; \
for f in $$(git diff --name-only HEAD 2>/dev/null) $$(git ls-files --others --exclude-standard 2>/dev/null); do \
cur_hash=$$(git hash-object "$$f" 2>/dev/null); \
prev_hash=$$(grep "^$$f " $(SNAPSHOT_FILE) 2>/dev/null | awk '{print $$2}'); \
if [ "$$cur_hash" != "$$prev_hash" ]; then \
new_dirty="$$new_dirty $$f"; \
fi; \
done; \
if [ $$rc -ne 0 ]; then \
echo "Step '$(1)' failed"; \
echo "$(1)" >> $(FAILURES_FILE); \
fi; \
if [ -n "$$new_dirty" ]; then \
echo "Files changed during '$(1)'. Run 'make $(1)' locally:"; \
echo "$$new_dirty" | tr ' ' '\n' | grep -v '^$$'; \
echo "$(1):dirty" >> $(FAILURES_FILE); \
fi
endef
endif

define check-failures
@if [ -s $(FAILURES_FILE) ]; then \
echo ""; \
echo "Failed steps:"; \
cat $(FAILURES_FILE); \
rm -f $(FAILURES_FILE); \
exit 1; \
else \
rm -f $(FAILURES_FILE); \
fi
endef

.PHONY: verify-ci
verify-ci: generate update staticcheck fmt vet verify-api-deps verify-crd-schema verify-docs-nav ## Run the same checks as the GHA verify workflow.
$(MAKE) verify-git-clean
verify-ci: ## Run the same checks as the GHA verify workflow.
@truncate -s0 $(FAILURES_FILE)
$(call run-step,generate)
$(call run-step,update)
$(call run-step,staticcheck)
$(call run-step,fmt)
$(call run-step,vet)
$(call run-step,verify-api-deps)
$(call run-step,verify-crd-schema)
$(call run-step,verify-docs-nav)
$(call check-failures)
Comment on lines 239 to +250

@coderabbitai coderabbitai Bot Jul 22, 2026

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Dropping the explicit verify-git-clean call narrows the "clean tree" guarantee.

Per the AI summary, the previous verify-ci recipe ended with $(MAKE) verify-git-clean, which checks the entire git status (staged/unstaged diffs, untracked files) regardless of cause. The new per-step dirty-detection in run-step only flags files that change during a given step's snapshot window — it will not catch files that were already modified/untracked before verify-ci started (e.g. a dirty local checkout, or stale artifacts from a previous run). Since this target is documented as running "the same checks as the GHA verify workflow" and is also meant for local use, this is a real regression in coverage versus the old behavior.

🛡️ Proposed fix: restore the full-tree clean check as a tracked step 
 	$(call run-step,verify-docs-nav)
+	$(call run-step,verify-git-clean)
 	$(call check-failures)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.PHONY: verify-ci
verify-ci: generate update staticcheck fmt vet verify-api-deps verify-crd-schema verify-docs-nav ## Run the same checks as the GHA verify workflow.
$(MAKE) verify-git-clean
verify-ci: ## Run the same checks as the GHA verify workflow.
@truncate -s0 $(FAILURES_FILE)
$(call run-step,generate)
$(call run-step,update)
$(call run-step,staticcheck)
$(call run-step,fmt)
$(call run-step,vet)
$(call run-step,verify-api-deps)
$(call run-step,verify-crd-schema)
$(call run-step,verify-docs-nav)
$(call check-failures)
.PHONY: verify-ci
verify-ci: ## Run the same checks as the GHA verify workflow.
`@truncate` -s0 $(FAILURES_FILE)
$(call run-step,generate)
$(call run-step,update)
$(call run-step,staticcheck)
$(call run-step,fmt)
$(call run-step,vet)
$(call run-step,verify-api-deps)
$(call run-step,verify-crd-schema)
$(call run-step,verify-docs-nav)
$(call run-step,verify-git-clean)
$(call check-failures)
🧰 Tools
🪛 checkmake (0.3.2)

[warning] 229-229: Target body for "verify-ci" exceeds allowed length of 5 lines (10).

(maxbodylength)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 228 - 239, Update the verify-ci recipe to restore the
explicit full-tree cleanliness check after the existing run-step checks and
before check-failures, using the established verify-git-clean target so
pre-existing staged, unstaged, and untracked changes are detected.


.PHONY: verify
verify: generate update staticcheck fmt vet
Expand Down Expand Up @@ -192,6 +279,9 @@ $(MOCKGEN): ${TOOLS_DIR}/go.mod
$(VERIFY_API_DEPS): $(TOOLS_DIR)/go.mod # Build verify-api-deps tool
cd $(TOOLS_DIR); $(GO) build -o $(BIN_DIR)/verify-api-deps ./verify-api-deps

$(GOTESTSUM): $(TOOLS_DIR)/go.mod # Build gotestsum from tools folder.
cd $(TOOLS_DIR); $(GO) build -tags=tools -o $(BIN_DIR)/gotestsum gotest.tools/gotestsum

$(CRD_SCHEMA_CHECK): $(TOOLS_DIR)/go.mod # Build crd-schema-check tool
cd $(TOOLS_DIR); $(GO) build -o $(BIN_DIR)/crd-schema-check ./crd-schema-check

Expand Down Expand Up @@ -406,10 +496,22 @@ test-changed:
# Usage: make test-shard TEST_PACKAGES="./cmd/... ./support/..." COVER_PROFILE="cover-shard.out"
TEST_PACKAGES ?= ./...
COVER_PROFILE ?= cover.out
ifdef GITHUB_ACTIONS
GOTESTSUM_FORMAT ?= github-actions
else
GOTESTSUM_FORMAT ?= pkgname
endif

.PHONY: run-tests
run-tests: $(GOTESTSUM)
GO111MODULE=on GOWORK=off GOFLAGS=-mod=vendor $(GOTESTSUM) --format=$(GOTESTSUM_FORMAT) -- $(GO_TEST_FLAGS) -parallel=$(NUM_CORES) -count=1 -timeout=30m $(TEST_PACKAGES) -coverprofile $(COVER_PROFILE)

.PHONY: test-shard
test-shard: generate
@echo "Running shard tests for packages: $(TEST_PACKAGES)"
$(GO) test $(GO_TEST_FLAGS) -parallel=$(NUM_CORES) -count=1 -timeout=30m $(TEST_PACKAGES) -coverprofile $(COVER_PROFILE)
test-shard:
@truncate -s0 $(FAILURES_FILE)
$(call run-step,generate)
$(call run-step,run-tests)
$(call check-failures)
Comment on lines +499 to +514

@coderabbitai coderabbitai Bot Jul 22, 2026

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does GitHub Actions correctly render nested ::group:: log sections, or does it break/confuse the collapsible UI?

💡 Result:

GitHub Actions does not natively support nested::group:: log sections [1]. If you attempt to nest groups by placing one::group::...::endgroup:: block inside another, the GitHub Actions log UI often fails to render them correctly [2][3]. Specifically, the UI logic typically treats the first::endgroup:: it encounters as the closing marker for the outermost (parent) group, regardless of how many groups were opened [2]. This causes the collapsible sections to behave unexpectedly—often breaking the folding functionality for the parent group or causing the nested structure to flatten and render incorrectly [2][3]. Key technical points regarding this limitation: 1. No Hierarchical Support: The workflow commands::group:: and::endgroup:: do not accept identifiers or keys, meaning the system has no way to match a specific "end" command to its corresponding "start" command [4]. 2. UI Parsing: The log viewer parses these markers linearly. Because there is no pairing mechanism, the renderer cannot maintain a proper tree-view or hierarchical state for nested sections [2][4]. 3. Recommended Practice: Official documentation and established community guidelines advise against nesting log groups [1]. You should maintain a flat structure of independent, sequential groups [1]. While GitHub has received numerous requests to support hierarchical or named log groups, it remains an unsupported feature [2][4][3]. Note that while the system internally uses different, non-user-accessible markers (like ##[start-action] and ##[end-action]) to render collapsible steps for composite actions, these cannot be triggered by user-defined script commands [5].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant Makefile sections and references to run-step / GOTESTSUM_FORMAT.
git ls-files Makefile .github/workflows | sed -n '1,200p'
printf '\n--- Makefile excerpt ---\n'
sed -n '150,220p;490,530p' Makefile
printf '\n--- References ---\n'
rg -n "GOTESTSUM_FORMAT|run-step|github-actions|pkgname|standard-verbose" Makefile .github/workflows -n

Repository: openshift/hypershift

Length of output: 7360

Avoid nested GitHub Actions log groups in run-tests.
run-step already wraps test-shard in a ::group::, so GOTESTSUM_FORMAT=github-actions adds another layer of groups and the Actions log UI can collapse them incorrectly. Use a non-grouping format like pkgname or standard-verbose here, or skip the outer group for run-tests.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 499 - 514, Update the test-shard execution flow around
run-tests and test-shard so GitHub Actions does not create nested log groups:
when GITHUB_ACTIONS is enabled, use a non-grouping GOTESTSUM_FORMAT such as
pkgname or standard-verbose, while preserving the existing format for other
environments and the run-step wrapper.


# OCP envtest index for downstream kubebuilder assets
ENVTEST_OCP_INDEX := https://raw.githubusercontent.com/openshift/api/master/envtest-releases.yaml
Expand Down
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestReconcileService(t *testing.T) {
Ports: []corev1.ServicePort{
{
Protocol: corev1.ProtocolTCP,
Port: 1125,
Port: 9999,

@coderabbitai coderabbitai Bot Jul 22, 2026

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.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix the expected port mismatch.

This test passes apiServerPort: 1125, and ReconcileService assigns that value directly to ServicePort.Port; therefore the result is 1125, not 9999. Restore the expectation or update the fixture input to 9999 if that is the intended contract.

Proposed fix 
-						Port:       9999,
+						Port:       1125,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Port: 9999,
Port: 1125,
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go` at
line 97, Correct the expected ServicePort.Port value in the ReconcileService
test to match the apiServerPort fixture input of 1125, unless the intended
contract is 9999, in which case update that fixture input accordingly. Keep the
test expectation and ReconcileService behavior consistent.

TargetPort: intstr.IntOrString{Type: intstr.String, StrVal: "client"},
},
},
Expand Down
2 changes: 1 addition & 1 deletion support/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
"crypto/tls"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"hash/fnv"
"io"
"io"
"net/http"
"os"
"regexp"

Check failure on line 16 in support/util/util.go

View workflow job for this annotation

GitHub Actions / Call main verify workflow / Verify

make fmt 

Not up to date. Run 'make fmt' locally and commit the result.
"sort"
"strings"

Expand Down
Loading