NO-JIRA: migrate ocp-53906 from otp to cvo repo#1347
NO-JIRA: migrate ocp-53906 from otp to cvo repo#1347jiajliu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds a new CVO e2e test that verifies ClusterVersion architecture info from the release image versus cluster nodes, extends the OC client with Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant OC as OC client
participant Registry as Release Image/Registry
participant API as API Server (config/v1 ClusterVersion)
participant Nodes as API Server (core/v1 Nodes)
Test->>OC: Extract pull-secret (resource/namespace --to)
OC-->>Test: pull-secret path
Test->>OC: AdmReleaseInfo (--insecure -a pullSecret -o json [--image])
OC->>Registry: fetch release metadata
Registry-->>OC: release JSON (includes release.openshift.io/architecture)
OC-->>Test: release JSON
Test->>API: Get ClusterVersion
API-->>Test: ClusterVersion ReleaseAccepted condition/message
Test->>Nodes: List Nodes
Nodes-->>Test: Node architectures
Test->>Test: Compare release arch vs node archs and assert CV message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jiajliu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/cvo/cvo.go`:
- Around line 122-162: The release architecture extracted into releaseArch must
be required, not treated as best-effort: after parsing releaseInfo (the
releaseArch variable populated from the nested releaseInfo map) add an assertion
that releaseArch is non-empty (fail fast via o.Expect) so missing metadata fails
the test; then in the non-heterogeneous branch (where you currently check node
architectures and compute expectedArch) assert that releaseArch matches
expectedArch (compare strings or containment against expectedArch) before
asserting cvArchInfo contains the architecture, and use the symbols releaseArch,
heterogeneousArchKeyword, expectedArch and cvArchInfo to locate where to add
these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19670f68-e7a7-4b99-b112-f2adb001844c
📒 Files selected for processing (4)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/oc/api/api.gotest/oc/cli/cli.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
130-133: ValidateExtractOptionsbefore building the command.At Line 131, empty
Resource/Namespace/Tovalues fall through toocand produce less actionable errors. Add a small guard for faster failure.Proposed diff
func (c *client) Extract(o api.ExtractOptions) error { + if o.Resource == "" || o.Namespace == "" || o.To == "" { + return fmt.Errorf("extract options must include resource, namespace, and to") + } args := []string{"extract", o.Resource, "-n", o.Namespace, "--confirm", fmt.Sprintf("--to=%s", o.To)} _, err := c.executor.Run(args...) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/oc/cli/cli.go` around lines 130 - 133, Validate the incoming api.ExtractOptions fields in client.Extract before constructing the oc args: check that o.Resource, o.Namespace and o.To are non-empty and return a clear error immediately if any are missing so we fail fast instead of invoking c.executor.Run with invalid args; update the Extract method (function client.Extract) to perform these guards and return an explanatory error message naming the missing field(s) before calling c.executor.Run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/oc/cli/cli.go`:
- Around line 130-133: Validate the incoming api.ExtractOptions fields in
client.Extract before constructing the oc args: check that o.Resource,
o.Namespace and o.To are non-empty and return a clear error immediately if any
are missing so we fail fast instead of invoking c.executor.Run with invalid
args; update the Extract method (function client.Extract) to perform these
guards and return an explanatory error message naming the missing field(s)
before calling c.executor.Run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33ec5fde-18e8-4b58-af96-5cce30d23827
📒 Files selected for processing (4)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/oc/api/api.gotest/oc/cli/cli.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
95-128: Implementation looks solid for test purposes.The method properly handles:
- Temp directory lifecycle with deferred cleanup
- Error propagation with descriptive messages
- Optional image argument
One minor observation: the method assumes the pull-secret will always extract to
.dockerconfigjson. While this is the standard filename for thepull-secretinopenshift-config, you might consider adding a file existence check before passing it tooc adm release infofor clearer error messages if extraction produces unexpected results.💡 Optional: Add file existence check
// Use the extracted pull-secret pullSecret := filepath.Join(tempDir, ".dockerconfigjson") + if _, err := os.Stat(pullSecret); err != nil { + return "", fmt.Errorf("pull-secret file not found at %s: %w", pullSecret, err) + } args := []string{"adm", "release", "info", "-a", pullSecret, "--insecure", "-o", "json"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/oc/cli/cli.go` around lines 95 - 128, Add a file-existence check for the extracted pull secret before invoking oc: after calling c.Extract(...) and computing pullSecret := filepath.Join(tempDir, ".dockerconfigjson"), verify that the file exists and is readable (os.Stat or equivalent) and return a descriptive error (including tempDir and expected filename) if it does not; keep the rest of AdmReleaseInfo (the tempDir cleanup, Extract call, and executor.Run invocation) unchanged and reference pullSecret in the error message so failures are easier to debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/oc/cli/cli.go`:
- Around line 95-128: Add a file-existence check for the extracted pull secret
before invoking oc: after calling c.Extract(...) and computing pullSecret :=
filepath.Join(tempDir, ".dockerconfigjson"), verify that the file exists and is
readable (os.Stat or equivalent) and return a descriptive error (including
tempDir and expected filename) if it does not; keep the rest of AdmReleaseInfo
(the tempDir cleanup, Extract call, and executor.Run invocation) unchanged and
reference pullSecret in the error message so failures are easier to debug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e86256b9-1cfd-45a3-a225-0b1adeb18e15
📒 Files selected for processing (4)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/oc/api/api.gotest/oc/cli/cli.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/cvo/cvo.go
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
134-137: ValidateExtractOptionsfields before invokingoc.Fail fast on missing required fields to return clearer errors than downstream CLI failures.
Proposed refactor
func (c *client) Extract(o api.ExtractOptions) error { + if strings.TrimSpace(o.Resource) == "" || strings.TrimSpace(o.Namespace) == "" || strings.TrimSpace(o.To) == "" { + return fmt.Errorf("resource, namespace, and to are required") + } args := []string{"extract", o.Resource, "-n", o.Namespace, "--confirm", fmt.Sprintf("--to=%s", o.To)} _, err := c.executor.Run(args...) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/oc/cli/cli.go` around lines 134 - 137, The Extract method currently invokes oc without validating required fields, causing unclear downstream CLI errors; update client.Extract to validate api.ExtractOptions (check o.Resource, o.Namespace and o.To are non-empty) and return a descriptive error immediately if any are missing instead of calling c.executor.Run, then proceed to build args and call c.executor.Run only when all required fields are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cvo/cvo.go`:
- Around line 119-122: The OC client is created without a Timeout which lets
AdmReleaseInfo use the short 30s default and causes flakiness; update the
oc.NewOC call to pass a longer timeout in ocapi.Options (e.g., set Timeout to a
larger duration) so ocClient := oc.NewOC(ocapi.Options{Logger: logger, Timeout:
<suitable duration>}) ensures AdmReleaseInfo has enough time to complete; modify
the NewOC invocation where oc.NewOC and ocapi.Options are used and keep the rest
of the call to AdmReleaseInfo unchanged.
---
Nitpick comments:
In `@test/oc/cli/cli.go`:
- Around line 134-137: The Extract method currently invokes oc without
validating required fields, causing unclear downstream CLI errors; update
client.Extract to validate api.ExtractOptions (check o.Resource, o.Namespace and
o.To are non-empty) and return a descriptive error immediately if any are
missing instead of calling c.executor.Run, then proceed to build args and call
c.executor.Run only when all required fields are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c2e1a32-200e-402f-a283-b03789443da7
📒 Files selected for processing (4)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/oc/api/api.gotest/oc/cli/cli.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
|
Test result on OCP: Test result on Hypershift: cc @hongkailiu @DavidHurta to review, thanks |
jiajliu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@jiajliu: 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. |
|
/retest-required |
|
@jiajliu: 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
This PR migrates test case 53906 from openshift-tests-private into cluster-version-operator, which validates CVO correctly parses release architecture into ClusterVersion status. Additionally, it adds the oc helper function AdmReleaseInfo() to support the test.