NO-JIRA: Introduce oc-cli for e2e tests #1267
Conversation
|
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 You can disable this status message by setting the WalkthroughAdds a testable OC client (api + CLI), a factory Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (2)
test/oc/cli.go (1)
20-90: Exec wrapper is sound; consider adding context/timeout and richer errorsThe
ocExecutor/executorlayering looks solid: you avoid shell invocation, keep args structured, and centralize logging around a singleRunentrypoint. For e2e use this is generally safe.Two improvements worth considering (not blockers):
- Add a timeout or context-aware path (e.g., using a variant that can call
exec.CommandContext) so long-runningoc adm release extractcalls can’t hang tests indefinitely.- Wrap the
LookPath("oc")andexecuteerrors with context (e.g., include the resolvedocpath and args in the returned error) so callers get actionable information even without digging into logs.test/oc/oc.go (1)
5-21: OC abstraction is clear; you can optionally drop the duplicate internal interfaceThe exported
OCinterface and options types make the test helper easy to consume. Right nowOChere andCliincli.gohave the same methods; if you don’t foresee them diverging, you could have the client implementOCdirectly and reuse that incli.goto avoid maintaining two parallel interfaces. Not required, but it would slightly reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/cvo/cvo.go(1 hunks)test/oc/cli.go(1 hunks)test/oc/oc.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/oc/cli.gotest/oc/oc.gotest/cvo/cvo.go
🧬 Code graph analysis (2)
test/oc/cli.go (1)
test/oc/oc.go (2)
ReleaseExtractOptions(5-7)VersionOptions(9-11)
test/cvo/cvo.go (2)
test/oc/oc.go (2)
NewOC(19-21)VersionOptions(9-11)pkg/version/version.go (1)
Version(16-16)
hongkailiu
force-pushed
the
oc-for-test
branch
from
f5eec0b to
8ea9560
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
82-98: Thin wrappers over oc commands look correctBoth
AdmReleaseExtractandVersionbuild the expected argument lists and delegate to the executor; the signatures match theapi.OCinterface, and returning the rawversionoutput as a string is appropriate for the current tests (e.g., checking for"Client Version:").If you later need more structured assertions, consider parsing
oc version -o jsoninstead of relying on string matching, but that’s not necessary for this initial usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json(1 hunks)go.mod(1 hunks)test/cvo/cvo.go(1 hunks)test/oc/api/api.go(1 hunks)test/oc/cli/cli.go(1 hunks)test/oc/oc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/oc/oc.go
- test/cvo/cvo.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/oc/api/api.gogo.modtest/oc/cli/cli.go
🧬 Code graph analysis (2)
test/oc/api/api.go (2)
pkg/cincinnati/cincinnati.go (1)
Client(33-43)pkg/version/version.go (1)
Version(16-16)
test/oc/cli/cli.go (2)
test/oc/api/api.go (3)
OC(11-14)ReleaseExtractOptions(3-5)VersionOptions(7-9)pkg/version/version.go (1)
Version(16-16)
🔇 Additional comments (5)
go.mod (1)
8-8: Direct logr dependency aligns with new logging usagePromoting
github.com/go-logr/logr v1.4.2to a direct requirement is appropriate now that it’s used explicitly (e.g., in the OC CLI wrapper). No issues from a maintainability or clarity standpoint.Please confirm this version matches the one expected by your logging setup and CI toolchain (e.g., no conflicting transitive versions from controller-runtime or other deps).
test/oc/api/api.go (1)
1-14: OC interface and option types are clean and cohesiveThe
OCinterface plusReleaseExtractOptions/VersionOptionsprovide a small, focused surface that cleanly matches the operations implemented in the CLI client. This should keep the test helpers easy to evolve without leaking implementation details.test/oc/cli/cli.go (2)
16-43: Executor abstraction and logging are well‑factoredInjecting an
executorplus theexecutefunc onocExecutoris a solid design for testability and future extension. The logging aroundRun(including command and args) is clear and should make diagnosing failures straightforward without overcomplicating the client.
64-80: Constructor behavior is straightforward; consider oc absence handling upstream
NewOCCliwiring viaexec.LookPath("oc")andnewOCExecutoris straightforward and keeps this package focused. Error propagation is clear. Just ensure callers/tests either skip or clearly fail whenocis not present on the test host, since this helper will return an error in that case..openshift-tests-extension/openshift_payload_cluster-version-operator.json (1)
11-21: The repository clone failed, preventing direct codebase verification. I cannot access the test files to confirm whether the test name exactly matches the actual Ginkgo test identifier or assess the test's stability for blocking lifecycle designation.
Verify test name alignment and blocking lifecycle status
The payload entry's structural validity is sound, but two aspects require verification from the test definition itself:
- The
"name"string must exactly match the Ginkgo/OC test identifier in the cluster-version-operator test suite; misalignment will prevent proper test selection.- The
"lifecycle": "blocking"status should be confirmed as appropriate for this test—verify the test is stable, handlesocunavailability gracefully, and does not introduce fragility in payload promotion workflows.These verifications require access to the cluster-version-operator test source code.
hongkailiu
force-pushed
the
oc-for-test
branch
from
8ea9560 to
b167bd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
46-62: Preserve timeout error type and any output instead of discarding itCurrent timeout handling replaces the original
context.DeadlineExceededwith a generic error and drops any collected output:o, err := c.CombinedOutput() if errors.Is(ctx.Err(), context.DeadlineExceeded) { return nil, fmt.Errorf("execution was terminated due to timeout") } return o, errThis makes it harder to:
- Detect timeouts programmatically (you lose
errors.Is(err, context.DeadlineExceeded)), and- See any partial output oc may have produced before timing out.
You can keep behavior the same for callers while improving debuggability by returning
oand wrapping the timeout error:- o, err := c.CombinedOutput() - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - return nil, fmt.Errorf("execution was terminated due to timeout") - } - - return o, err + o, err := c.CombinedOutput() + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + // Preserve any partial output and wrap the underlying timeout error + return o, fmt.Errorf("execution timed out after %s: %w", timeout, ctx.Err()) + } + return o, errCallers still just see a non-nil error on timeout, but you retain the original timeout type (for
errors.Is) and any output in both logs and return value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json(1 hunks)go.mod(1 hunks)test/cvo/cvo.go(1 hunks)test/oc/api/api.go(1 hunks)test/oc/cli/cli.go(1 hunks)test/oc/oc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- test/oc/oc.go
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/oc/api/api.gotest/oc/cli/cli.gotest/cvo/cvo.go
🧬 Code graph analysis (3)
test/oc/api/api.go (1)
pkg/version/version.go (1)
Version(16-16)
test/oc/cli/cli.go (1)
test/oc/api/api.go (3)
OC(11-14)ReleaseExtractOptions(3-5)VersionOptions(7-9)
test/cvo/cvo.go (2)
test/oc/oc.go (1)
NewOC(11-13)test/oc/api/api.go (1)
VersionOptions(7-9)
🔇 Additional comments (2)
test/cvo/cvo.go (1)
11-26: Oc-based e2e test wiring looks goodLogger setup and the
oc.NewOC(logger)+Version(VersionOptions{Client: true})flow are consistent with the new oc client API, and the expectations are clear and appropriately strict for an environment whereocis required.test/oc/api/api.go (1)
1-14: OC API surface is minimal and cohesive for testsThe option structs and
OCinterface cleanly model the two supported operations (adm release extractandversion) without over-design, and they match how the client is used in tests and the CLI wrapper.
hongkailiu
force-pushed
the
oc-for-test
branch
from
b167bd2 to
d5d585d
Compare
hongkailiu
force-pushed
the
oc-for-test
branch
from
d5d585d to
f4c8d4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json(1 hunks)go.mod(1 hunks)test/cvo/cvo.go(1 hunks)test/oc/api/api.go(1 hunks)test/oc/cli/cli.go(1 hunks)test/oc/oc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/cvo/cvo.go
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/oc/api/api.gotest/oc/oc.gotest/oc/cli/cli.go
🧬 Code graph analysis (3)
test/oc/api/api.go (1)
pkg/version/version.go (1)
Version(16-16)
test/oc/oc.go (2)
test/oc/api/api.go (1)
OC(11-14)test/oc/cli/cli.go (1)
NewOCCli(66-89)
test/oc/cli/cli.go (1)
test/oc/api/api.go (3)
OC(11-14)ReleaseExtractOptions(3-5)VersionOptions(7-9)
🔇 Additional comments (4)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json (1)
11-21: LGTM!The new test entry is properly structured and includes all required fields for payload test registration.
test/oc/oc.go (1)
10-13: LGTM!Clean delegation pattern that provides a clear API boundary between the public interface and implementation details.
test/oc/api/api.go (1)
3-14: LGTM!Clean API design using the options pattern for extensibility. The interface is minimal and well-focused on test requirements.
test/oc/cli/cli.go (1)
65-89: LGTM!The 30s default timeout and configurable
OC_CLI_TIMEOUTenvironment variable address the previous concern about hard-coded timeouts. The implementation properly handles binary lookup, timeout parsing, and error propagation.
hongkailiu
force-pushed
the
oc-for-test
branch
2 times, most recently
from
db03355 to
ae3f3f5
Compare
Most of e2e tests for CVO use `oc` cmds that are not trivial to implement or import. See the introduced `oc/api/api.go`. This pull introduces the implementation by spawning processes to execute `oc` cmd. The advantages are: - We are ready to add more e2e tests that need `oc` cmds. - We do not need to vendor any new pkgs into CVO, e.g., `k8s.io/kubernetes/test/e2e/framework` which might give us headaches when bumping its version. - The implementation can be easily replaced when needed in the future with a better one without oc-cli because the `test/oc/cli` pkg stays only in the `test/oc` pkg, and the tests in a pkg such as `/test/cvo` does not know the existence of `cli`.
hongkailiu
force-pushed
the
oc-for-test
branch
from
ae3f3f5 to
77bffea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/oc/api/api.go (2)
3-9: Add documentation for exported option types.These exported types in an API package lack godoc comments. Adding brief documentation would improve maintainability and help future contributors understand the purpose of each field.
Example documentation:
+// ReleaseExtractOptions configures the 'oc adm release extract' command. type ReleaseExtractOptions struct { + // To specifies the target directory for extracted release content. To string } +// VersionOptions configures the 'oc version' command. type VersionOptions struct { + // Client requests only client version information when true. Client bool }
11-14: Add documentation for the OC interface.The exported
OCinterface should have a godoc comment explaining its purpose and the commands it wraps.+// OC provides an interface for executing oc CLI commands in e2e tests. +// Implementations spawn oc subprocesses to avoid vendoring additional dependencies. type OC interface { + // AdmReleaseExtract executes 'oc adm release extract' with the given options. AdmReleaseExtract(o ReleaseExtractOptions) error + // Version executes 'oc version' and returns the version output. Version(o VersionOptions) (string, error) }Optional: Consider adding
context.Contextparameters to support cancellation and timeouts:type OC interface { AdmReleaseExtract(ctx context.Context, o ReleaseExtractOptions) error Version(ctx context.Context, o VersionOptions) (string, error) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json(1 hunks)go.mod(1 hunks)test/cvo/cvo.go(1 hunks)test/oc/api/api.go(1 hunks)test/oc/cli/cli.go(1 hunks)test/oc/oc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/oc/oc.go
- test/cvo/cvo.go
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
- test/oc/cli/cli.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
go.modtest/oc/api/api.go
🧬 Code graph analysis (1)
test/oc/api/api.go (2)
pkg/cincinnati/cincinnati.go (1)
Client(33-43)pkg/version/version.go (1)
Version(16-16)
🔇 Additional comments (1)
go.mod (1)
8-8: Dependency promotion is well-justified.Promoting
github.com/go-logr/logrfrom an indirect to a direct dependency is appropriate given that the new oc test CLI components instantiate and pass logr.Logger instances directly. Since v1.4.2 was already locked as a transitive dependency, this change is safe and poses no risk of version skew.
|
@hongkailiu: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| execute func(dir, command string, args ...string) ([]byte, error) | ||
| } | ||
|
|
||
| func (e *ocExecutor) Run(args ...string) ([]byte, error) { |
There was a problem hiding this comment.
Why not implement Run on client directly?
In this PR, you introduced ocExecutor struct and executor interface, it looks overly complicated.
There was a problem hiding this comment.
It is my way/habit to layer things up.
When we have another ocExecutor implementation, we could hook it up easily.
Let us see if everyone is OK with this way, building up oc's features.
I can make it flat later (not a big deal to me) when the idea is accepted.
From my experience, we do not need to touch this part of code any more once merged.
All we need to do is to add new functions to the interface if needed and call the oc with the right sequence of args to implement it.
There was a problem hiding this comment.
In my experience, interfaces should only be introduced when there are many different instances, but for the CVO testing, there is only one OC client (we will not use client-go), so the interfaces are not necessary.
Leave it for @DavidHurta @jhou1 @jiajliu @shahsahil264 to confirm as you will use it in the future.
|
/cc The oc-cli PR is on my watchlist. Bear with me while I finish some other tasks, please. I am intrigued by this approach, so I am looking forward to taking a look at it. |
|
I really like this overall approach. If there are no other secret dependencies in OTP, this is really great IMO. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, JianLi-RH The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @hongkailiu $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1267/pull-ci-openshift-cluster-version-operator-main-e2e-agnostic-ovn/1994507800205594624/artifacts/e2e-agnostic-ovn/openshift-e2e-test/artifacts/e2e.log | grep 'cluster-version-operator-tests can use'
started: 0/284/424 "[Jira:\"Cluster Version Operator\"] cluster-version-operator-tests can use oc to get the version information"
passed: (100ms) 2025-11-28T22:43:35 "[Jira:\"Cluster Version Operator\"] cluster-version-operator-tests can use oc to get the version information" |
openshift-ci-robot
added
the
verified
|
@hongkailiu: This PR has been marked as verified by In 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. |
hongkailiu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. In 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. |
openshift-ci
bot
added
the
do-not-merge/hold
|
overall approach LGTM and can be used for pure oc sub-command depended tests or commands. Thanks @hongkailiu for the PR. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
7 participants
Most of e2e tests for CVO use
occmds that are not trivialto implement or import. See the introduced
oc/api/api.go.This pull introduces the implementation by spawning processes
to execute
occmd.The advantages are:
occmds.k8s.io/kubernetes/test/e2e/frameworkwhich might give usheadaches when bumping its version.
the future with a better one without oc-cli because the
test/oc/clipkg stays only in thetest/ocpkg, andthe tests in a pkg such as
/test/cvodoes not knowthe existence of
cli.