NO-JIRA: More flexible timeout option for oc-cli

[hongkailiu]

Follow up #1309 (comment)

We made an option type for oc cmd.

Currently, it is mostly timeout for running the command. The current implementation use it as the timeout for the context when spawning the process.

The option also gives the flexibility to introduce other options for oc.

/cc @JianLi-RH

[coderabbitai]

Walkthrough

This change refactors the OC client initialization API to use an Options struct instead of direct parameter passing. A new Options type is introduced containing Logger and Timeout fields. Function signatures for NewOC and NewOCCli are updated to accept Options, with timeout configuration moving from environment variables to the Options.Timeout field with a 30-second default.

Changes

Cohort / File(s)	Summary
API Definition test/oc/api/api.go	Introduces new Options struct with Logger (logr.Logger) and Timeout (time.Duration) fields.
Client Constructors test/oc/oc.go, test/oc/cli/cli.go	Update NewOC and NewOCCli function signatures to accept api.Options instead of direct logger parameter; timeout configuration moved from environment variable to Options.Timeout with 30s default when zero.
Call Site Update test/cvo/cvo.go	Updates NewOC call to pass ocapi.Options{Logger: logger} struct instead of bare logger parameter.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[JianLi-RH]

/hold

Let's discuss it together, may be @jhou1 wants to join?

[JianLi-RH]

If we add Timeout in the structure function, the timeout setting will be fixed, that means we have to instantiate oc client for each case.

[hongkailiu]

that means we have to instantiate oc client for each case.

I do not see why for each case.

The oc with default timeout (30s). If you do not want to use it because the timeout is too short, you may create a new one for it. There are only a few cases like it, right?

Do you want a timeout for each cmd?
We could do it that way but I do not see the use cases.

[JianLi-RH]

I'd like to put the oc client instantiation scripts into g.BeforeEach(), like kubeClient, err = util.GetKubeClient(restCfg).
So for each test case which uses ocapi.OC will has same timeout setting, for the special case, like 42543 have to create a new ocapi.OC.

[hongkailiu]

I'd like to put the oc client instantiation scripts into g.BeforeEach()

I am fine with it but that is not really a blocker of this pull, right?
You can do it with a commit in #1309 after this one gets in.
I did not do it here because there is only one case using oc at the moment.
Moving it now inside BeforeEach looks odd to me.

What I did here is only making the API more easier to use: Parameters in an option vs Environment variable.

[openshift-ci-robot]

@hongkailiu: This pull request explicitly references no jira issue.

Details

In response to this:

Follow up #1309 (comment)

We made an option type for oc cmd.

Currently, it is mostly timeout for running the command. The current implementation use it as the timeout for the context when spawning the process.

The option also gives the flexibility to introduce other options for oc.

/cc @JianLi-RH

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.

[JianLi-RH]

/lgtm

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [hongkailiu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JianLi-RH]

We need to do more optimize to the timeout setting, but not urgent.

[hongkailiu]

We need to do more optimize to the timeout setting, but not urgent.

For future ourselves, what Jian wants is a setter to change the timeout for an existing oc instance.
For now the use cases are occasionally oc adm release extract needs more time than the default 30s.
However, this might lead to racing for parallel running cases.
We might seek for a solution for this as a future work.

[hongkailiu]

/hold cancel

[hongkailiu]

As far as I know, no tests uses the env. vars to configure the timeout.

/verified by @hongkailiu

[openshift-ci-robot]

@hongkailiu: This PR has been marked as verified by @hongkailiu.

Details

In response to this:

As far as I know, no tests uses the env. vars to configure the timeout.

/verified by @hongkailiu

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]

@hongkailiu: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.