fix(ci): add GHA structured output to verify-ci#9049
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Makefile now uses Bash, records verification failures, detects step-generated changes in GitHub Actions, and reports grouped annotations. CI verification and test shards use shared failure handling. Tests run through a built Sequence Diagram(s)sequenceDiagram
participant VerifyCI as verify-ci
participant RunStep as run-step
participant Target as verification target
participant CheckFailures as check-failures
VerifyCI->>RunStep: run named verification target
RunStep->>Target: execute target and record failures
RunStep->>RunStep: detect changed files under GitHub Actions
VerifyCI->>CheckFailures: evaluate recorded failures
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/verify.yamlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
174-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the
checkmakemax-body-length warning.
verify-cinow has nine recipe lines while the configured limit is five. Keep the required ordered execution, but move the step list into a helper construct or add a narrowly scoped, documented exemption.🤖 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 174 - 183, Reduce the recipe body of verify-ci to satisfy checkmake’s five-line limit while preserving the current ordered execution of all nine run-verify-step targets. Move the step list into a dedicated helper construct, or add a narrowly scoped documented exemption if that matches the Makefile’s existing conventions.Source: Linters/SAST tools
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@Makefile`:
- Around line 174-183: Reduce the recipe body of verify-ci to satisfy
checkmake’s five-line limit while preserving the current ordered execution of
all nine run-verify-step targets. Move the step list into a dedicated helper
construct, or add a narrowly scoped documented exemption if that matches the
Makefile’s existing conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8df14bea-c7eb-48ae-a9d2-a098e947c424
📒 Files selected for processing (1)
Makefile
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 61c15f0 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #9049 +/- ##
=======================================
Coverage 44.44% 44.44%
=======================================
Files 774 774
Lines 96977 96977
=======================================
Hits 43105 43105
Misses 50897 50897
Partials 2975 2975
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
20f81b2 to
c6e418d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go`:
- Around line 30-32: Update the test case using modifyKasConfig so the default
test compares against the valid defaultKASConfig() bind address and remains
green with empty parameters. Move the deliberately invalid
ServingInfo.BindAddress value into a separate opt-in or manual test/CI fixture
that specifically exercises failure annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 152fe08f-2797-4712-bb22-eebdb4557d58
📒 Files selected for processing (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/config_test.go
| expected: modifyKasConfig(defaultKASConfig(), func(kasc *kcpv1.KubeAPIServerConfig) { | ||
| kasc.ServingInfo.BindAddress = "DELIBERATELY_BROKEN_TO_TEST_GHA_OUTPUT" | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Do not commit a permanently failing default test.
With empty parameters, this case compares the generated config against a deliberately invalid bind address instead of defaultKASConfig()’s normal 0.0.0.0:0, so cmp.Diff will fail in every environment—not only GitHub Actions. Keep the production test green and exercise failure annotations with a separate opt-in/manual test or CI fixture.
🤖 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/v2/kas/config_test.go`
around lines 30 - 32, Update the test case using modifyKasConfig so the default
test compares against the valid defaultKASConfig() bind address and remains
green with empty parameters. Move the deliberately invalid
ServingInfo.BindAddress value into a separate opt-in or manual test/CI fixture
that specifically exercises failure annotations.
|
/test |
|
@deads2k: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ecc4742 to
61c15f0
Compare
openshift-ci
Bot
added
the
area/hypershift-operator
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Makefile (1)
165-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
run-steplogic acrossifdef/elsebranches.The two macro definitions are nearly identical (172-178 vs 197-203 for snapshotting/dirty-detection), differing only in the group/annotation echoes. Checking
$$GITHUB_ACTIONSat shell runtime instead ofifdef GITHUB_ACTIONSat Make-parse time would collapse this into a single definition and remove the duplication (also incidentally fixes the checkmake body-length warning onverify-ciby shrinking overall macro size).♻️ Proposed consolidation
-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)"; \ + if [ -n "$$GITHUB_ACTIONS" ]; then echo "::group::$(1)"; fi; \ $(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 "::error::Step '$(1)' failed"; \ + if [ -n "$$GITHUB_ACTIONS" ]; then echo "::error::Step '$(1)' failed"; else echo "Step '$(1)' failed"; fi; \ echo "$(1)" >> $(FAILURES_FILE); \ fi; \ if [ -n "$$new_dirty" ]; then \ - for f in $$new_dirty; do \ - echo "::error file=$$f::File changed during '$(1)'. Run 'make $(1)' locally."; \ - done; \ + if [ -n "$$GITHUB_ACTIONS" ]; then \ + for f in $$new_dirty; do echo "::error file=$$f::File changed during '$(1)'. Run 'make $(1)' locally."; done; \ + else \ + echo "Files changed during '$(1)'. Run 'make $(1)' locally:"; \ + echo "$$new_dirty" | tr ' ' '\n' | grep -v '^$$'; \ + fi; \ echo "$(1):dirty" >> $(FAILURES_FILE); \ - fi; \ - echo "::endgroup::" + fi; \ + if [ -n "$$GITHUB_ACTIONS" ]; then echo "::endgroup::"; fi endef -else -define run-step - ... -endef -endif🤖 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 165 - 214, Consolidate the duplicated run-step macro into one definition instead of separate ifdef GITHUB_ACTIONS branches. Preserve the shared snapshot, nested make, failure tracking, and dirty-file detection logic, while selecting the GitHub Actions grouping/error annotations versus local output through a shell-time GITHUB_ACTIONS check within run-step.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Makefile`:
- Around line 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.
- Around line 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.
- Around line 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.
In `@support/certs/tls_test.go`:
- Around line 229-232: Remove the unconditional failing TestDeliberatelyBroken
test from the test suite, or gate its failure behind an explicit opt-in
test-only mechanism so normal run-tests and check-failures executions pass.
---
Nitpick comments:
In `@Makefile`:
- Around line 165-214: Consolidate the duplicated run-step macro into one
definition instead of separate ifdef GITHUB_ACTIONS branches. Preserve the
shared snapshot, nested make, failure tracking, and dirty-file detection logic,
while selecting the GitHub Actions grouping/error annotations versus local
output through a shell-time GITHUB_ACTIONS check within run-step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 435fce14-f8ec-4942-97e4-5801c37bac25
📒 Files selected for processing (3)
Makefilesupport/certs/tls_test.gosupport/util/util.go
| FAILURES_FILE := $(shell mktemp) | ||
| SNAPSHOT_FILE := $(shell mktemp) |
There was a problem hiding this comment.
🩺 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.
| 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; \ |
There was a problem hiding this comment.
🎯 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-objecton the now-missing file fails silently (cur_hash=""), and since the file was never in$(SNAPSHOT_FILE)(it wasn't dirty pre-step),prev_hashis also""— the deletion is never flagged asnew_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.
| 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.
| .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) |
There was a problem hiding this comment.
🎯 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.
| .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.
| func TestDeliberatelyBroken(t *testing.T) { | ||
| t.Error("deliberately broken to test GHA output") | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the unconditional failing test before merge.
TestDeliberatelyBroken always fails, so run-tests records a failure and check-failures exits with status 1 on every CI run. Remove it after validating the annotation flow, or gate it behind an explicit opt-in test-only mechanism.
🤖 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 `@support/certs/tls_test.go` around lines 229 - 232, Remove the unconditional
failing TestDeliberatelyBroken test from the test suite, or gate its failure
behind an explicit opt-in test-only mechanism so normal run-tests and
check-failures executions pass.
61c15f0 to
10d9590
Compare
Add run-step Makefile macro that detects GITHUB_ACTIONS and wraps each step with ::group::/::endgroup:: for collapsible sections and ::error:: annotations on failure. Uses git hash-object to detect per-step content changes (not just filename presence), so if step A dirties a file and step B modifies it further, both steps report it. All steps run without fail-early; failures are aggregated and reported at the end. For test-shard, use gotestsum with github-actions format for per-test collapsible groups on failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
10d9590 to
0e2842a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go`:
- 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.
In `@Makefile`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b6eb54c1-4e90-4c3d-a62e-174a7781858e
📒 Files selected for processing (4)
.github/workflows/verify.yamlMakefilecontrol-plane-operator/controllers/hostedcontrolplane/kas/service_test.gosupport/util/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/util/util.go
| { | ||
| Protocol: corev1.ProtocolTCP, | ||
| Port: 1125, | ||
| Port: 9999, |
There was a problem hiding this comment.
🎯 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.
| 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.
| 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) |
There was a problem hiding this comment.
🎯 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:
- 1: https://github.com/PowerShell/PowerShell/blob/master/.github/instructions/log-grouping-guidelines.instructions.md
- 2: nesting group-endgroup workflow commands actions/runner#802
- 3: Nested logging groups tree-view actions/toolkit#1001
- 4: Support named log groups actions/runner#1477
- 5: Composite Action Step Markers actions/runner#4243
🏁 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 -nRepository: 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.
|
@devguyio: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
run-verify-stepMakefile function that detectsGITHUB_ACTIONSenv var::group::/::endgroup::for collapsible log sections and emits::error::on failure$(MAKE)calls, no change in behaviorverify-cifrom a prerequisite chain to explicit step calls so each step gets its own groupTest plan
make verify-cilocally produces plain output (no::markers)GITHUB_ACTIONS=true make verify-ciproduces::group::Verify: <step>/::endgroup::for each step::error::Verify step '<step>' failedis emitted after closing the group🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores