[WIP] Refactor test/extended/util/client.go
#28246
Conversation
| kubeConfig, _, err = GetHypershiftManagementClusterConfigAndNamespace() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| } else { | ||
| kubeConfig = KubeConfigPath() |
There was a problem hiding this comment.
CLIOptions should provide an indicator for
- use the global
- use this path
- use the hypershift thing above
There was a problem hiding this comment.
Yeah, probably this comment along with the other about using With* methods should go together.
test/extended/util/client.go
Outdated
| // KubeFramework to use, instead of creating a brand new one | ||
| KubeFramework *framework.Framework | ||
| // prefix for the created test namespace | ||
| BaseName string |
There was a problem hiding this comment.
seems like this and withoutnamespace should be mutually exclusive.
There was a problem hiding this comment.
Do we really need to set BaseName when withoutnamespace is true?
There was a problem hiding this comment.
BaseName is used as a UniqueName here: https://github.com/kubernetes/kubernetes/blob/24fbb13eafec665e6f4b64961930774babd8b6d1/test/e2e/framework/framework.go#L279-L282 we could not require it being set and allow it to be randomly generated when empty, especially that this f.UniqueName is used pretty heavily in some cases. So for now I'd prefer we just keep it in there 😉
There was a problem hiding this comment.
I've added that information in the field description for now.
test/extended/util/client.go
Outdated
|
|
||
| // NewCLI is responsible for creating a new CLI object for invoking OpenShift CLI, | ||
| // CliOptions allow customization of the created object. | ||
| func NewCLI(options CliOptions) *CLI { |
There was a problem hiding this comment.
Perhaps you want NewCLI(opts ...Option)
type Option func(cliOptions) cliOptions
func withTestFramework(framework) Option
func withPSALevel(level) Option
func withoutNamespace() OptionThere was a problem hiding this comment.
Good idea, especially that we already have a lot of these With* methods.
| } | ||
| _, err := c.AdminKubeClient().CoreV1().Namespaces().Create(context.Background(), nsObject, metav1.CreateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| c.kubeFramework.AddNamespacesToDelete(nsObject) |
There was a problem hiding this comment.
Is the kubeframework wired in some way that requires us to contain it instead of creating an entirely separate thign?
There was a problem hiding this comment.
We have a few cases where we modify the internals of the test framework, and I didn't want to address that in this PR.
| nc := *c | ||
| nc.configPath = c.adminConfigPath | ||
| return &nc | ||
| c.currentConfigPath = c.adminConfigPath |
There was a problem hiding this comment.
in case of multiple CLI modifications within a test, would it make sense to protect all these changes with mutexes?
There was a problem hiding this comment.
I'd leave it up to consumers to deal with that kind of changes. I can probably add a comment to NewCLI about not having these kind of guarantees.
| func (c CLI) WithToken(token string) *CLI { | ||
| c.configPath = "" | ||
| func (c *CLI) WithToken(token string) *CLI { | ||
| c.currentConfigPath = "" |
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| err = c.setupNamespacePodSecurity(newNamespace) |
There was a problem hiding this comment.
Why we don't need this anymore?. Because we are already setting it to restricted in somewhere?
There was a problem hiding this comment.
I'm squashing two methods into one, to ensure we always use the same code path, and clean all the resources after the tests.
test/extended/util/client.go
Outdated
| // KubeFramework to use, instead of creating a brand new one | ||
| KubeFramework *framework.Framework | ||
| // prefix for the created test namespace | ||
| BaseName string |
There was a problem hiding this comment.
Do we really need to set BaseName when withoutnamespace is true?
|
/refresh-status |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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/test-infra repository. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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/test-infra repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh 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
soltysh
removed
needs-rebase
|
Job Failure Risk Analysis for sha: cbcf49d
|
|
@soltysh: The following tests failed, say
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. |
|
Job Failure Risk Analysis for sha: 2d3fb70
|
6 participants
This PR does the following things at a high level:
test/extended/util/client.goto get rid of duplicate code and ensure all creation goes through the exact same path - singleNewCLImethod with a structure that allows you to configure the necessary variant