NO-ISSUE: Make default cluster version setup idempotent in IT - #260
NO-ISSUE: Make default cluster version setup idempotent in IT#260jhernand wants to merge 1 commit into
Conversation
Move default cluster version creation from the test suite BeforeSuite hook into Tool.Setup and treat AlreadyExists as success. This keeps integration tests from failing on a subsequent run when the service is left running after the previous run, for example with IT_KEEP_SERVICE. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
@jhernand: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand 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 |
WalkthroughThe integration setup now ensures the hub and default ChangesIntegration setup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🤖 Finished Review · ✅ Success · Started 12:42 PM UTC · Completed 12:57 PM UTC Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@fulfillment-service/it/it_tool.go`:
- Around line 2338-2342: Update the cluster-version creation flow around
client.Create to return nil immediately when creation succeeds, before
converting the error status. Preserve the existing AlreadyExists handling for
failed creation and keep wrapping other errors with the existing context; cover
both success and AlreadyExists paths.
🪄 Autofix
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e945930-19bf-4e65-b200-445d965fb9ca
📒 Files selected for processing (2)
fulfillment-service/it/it_suite_test.gofulfillment-service/it/it_tool.go
💤 Files with no reviewable changes (1)
- fulfillment-service/it/it_suite_test.go
| status, ok := grpcstatus.FromError(err) | ||
| if ok && status.Code() == grpccodes.AlreadyExists { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to create default cluster version: %w", err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '2323,2343p' fulfillment-service/it/it_tool.goRepository: osac-project/osac
Length of output: 1020
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("fulfillment-service/it/it_tool.go")
lines = p.read_text().splitlines()
for start, end in [(2323, 2343)]:
print(f"--- {p}:{start}-{end} ---")
for n in range(start, end + 1):
print(f"{n}: {lines[n-1]}")
for name in ("fulfillment-service/go.mod", "go.mod"):
q = Path(name)
if q.exists():
print(f"--- {q} ---")
for line in q.read_text().splitlines():
if line.startswith("go ") or "grpc" in line:
print(line)
PY
go_version="$(go version 2>/dev/null || true)"
printf '%s\n' "$go_version"
if command -v go >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
grpcodes "google.golang.org/grpc/codes"
grpcstatus "google.golang.org/grpc/status"
)
func main() {
var err error
status, ok := grpcstatus.FromError(err)
fmt.Printf("FromError(nil): status_nil=%v ok=%v code=%v\n", status == nil, ok, status.Code())
fmt.Printf("fmt.Errorf: nil_error=%v\n", fmt.Errorf("failed: %w", err) == nil)
alreadyExists := grpcstatus.Error(grpcodes.AlreadyExists, "exists")
status, ok = grpcstatus.FromError(alreadyExists)
fmt.Printf("AlreadyExists: status_nil=%v ok=%v code=%v\n", status == nil, ok, status.Code())
}
EOF
(
cd "$tmpdir"
GO111MODULE=on go mod init verifier >/dev/null
go mod edit -require=google.golang.org/grpc@v1.83.0
go run .
)
fiRepository: osac-project/osac
Length of output: 1736
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("fulfillment-service/it/it_tool.go")
lines = p.read_text().splitlines()
for n in range(2323, 2344):
print(f"{n}: {lines[n-1]}")
for name in ("fulfillment-service/go.mod", "go.mod"):
q = Path(name)
if q.exists():
print(f"--- {q} ---")
for line in q.read_text().splitlines():
if line.startswith("go ") or "grpc" in line:
print(line)
PY
go version 2>/dev/null || true
if command -v go >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
grpcodes "google.golang.org/grpc/codes"
grpcstatus "google.golang.org/grpc/status"
)
func main() {
var err error
status, ok := grpcstatus.FromError(err)
fmt.Printf("FromError(nil): status_nil=%v ok=%v code=%v\n", status == nil, ok, status.Code())
fmt.Printf("fmt.Errorf: nil_error=%v\n", fmt.Errorf("failed: %w", err) == nil)
alreadyExists := grpcstatus.Error(grpcodes.AlreadyExists, "exists")
status, ok = grpcstatus.FromError(alreadyExists)
fmt.Printf("AlreadyExists: status_nil=%v ok=%v code=%v\n", status == nil, ok, status.Code())
}
EOF
(
cd "$tmpdir"
GO111MODULE=on go mod init verifier >/dev/null
go mod edit -require=google.golang.org/grpc@v1.83.0
go run .
)
fiRepository: osac-project/osac
Length of output: 1684
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import "fmt"
func main() {
var err error
wrapped := fmt.Errorf("failed to create default cluster version: %w", err)
fmt.Printf("fmt.Errorf with nil operand: is_nil=%v, text=%q\n", wrapped == nil, wrapped.Error())
}
EOF
(
cd "$tmpdir"
go run main.go
)
curl -fsSL \
https://raw.githubusercontent.com/grpc/grpc-go/v1.83.0/status/status.go |
awk '
/func FromError\(err error\)/ {show=1}
show {print}
show && /^}/ {exit}
'
curl -fsSL \
https://raw.githubusercontent.com/grpc/grpc-go/v1.83.0/status/status.go |
awk '
/func \(s \*Status\) Code\(\)/ {show=1}
show {print}
show && /^}/ {exit}
'Repository: osac-project/osac
Length of output: 1268
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- references ---'
rg -n -C 3 'ensureDefaultClusterVersion|Create\(ctx.*ClusterVersions|AlreadyExists' fulfillment-service/it \
-g '*.go' | head -240
printf '%s\n' '--- grpc Status.Code implementation ---'
curl -fsSL \
https://raw.githubusercontent.com/grpc/grpc-go/v1.83.0/status/status.go |
rg -n -A 8 -B 3 'func \(s \*Status\) Code|func Code\('
printf '%s\n' '--- test files and outlines ---'
git ls-files 'fulfillment-service/it/*_test.go'
for f in $(git ls-files 'fulfillment-service/it/*_test.go' | head -20); do
printf '%s\n' "--- $f ---"
ast-grep outline "$f" 2>/dev/null | head -80 || true
doneRepository: osac-project/osac
Length of output: 19469
Return nil when cluster-version creation succeeds.
When client.Create succeeds, the AlreadyExists branch does not run. fmt.Errorf(..., %w) still returns a non-nil error, so Tool.Setup fails after creating the resource. Return nil before status conversion, retain the existing AlreadyExists handling, and cover both paths.
🤖 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 `@fulfillment-service/it/it_tool.go` around lines 2338 - 2342, Update the
cluster-version creation flow around client.Create to return nil immediately
when creation succeeds, before converting the error status. Preserve the
existing AlreadyExists handling for failed creation and keep wrapping other
errors with the existing context; cover both success and AlreadyExists paths.
ReviewFindingsCritical
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| }.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| status, ok := grpcstatus.FromError(err) |
There was a problem hiding this comment.
[critical] error handling
ensureDefaultClusterVersion always returns a non-nil error on successful creation. When client.Create succeeds (err is nil), grpcstatus.FromError(nil) returns (OK status, true), so the AlreadyExists check fails, and execution falls through to return fmt.Errorf("failed to create default cluster version: %w", err) where err is nil. fmt.Errorf with a nil %w produces a non-nil error, which causes Tool.Setup to abort. Integration tests will fail on first run when no pre-existing cluster version exists.
Suggested fix: Add a nil check for err before inspecting the gRPC status, matching the pattern used in ensureHub: wrap the status check inside if err != nil { ... } and add a final return nil.
2 participants
Summary
Move default cluster version creation from the test suite
BeforeSuitehook intoTool.Setupand treatAlreadyExistsas success. This keeps integration testsfrom failing on a subsequent run when the service is left running after the
previous run, for example with
IT_KEEP_SERVICE.Test plan
IT_KEEP_SERVICEset and verify setup succeedsSummary by CodeRabbit