Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e:hypershift: helper functions #1078

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Nov 18, 2024

Adding some helper packages for interacting with NodePools object and talking to the HyperShift MNG cluster.
This work inspired by similar work done on: https://github.com/openshift/cluster-node-tuning-operator

vendor hypershift API needed for test utils implementation.

Signed-off-by: Talor Itzhak <[email protected]>
Copy link
Contributor

openshift-ci bot commented Nov 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Tal-or

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 18, 2024

/hold
depends on #openshift/release#58789

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2024
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem I have with this code is just one:
if we add in test/utils adding tests becomes unnecessarily hard. It would be much better to add code in internal/ and use it (only) in tests. How much of this code can we move in internal/?

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 18, 2024

if we add in test/utils adding tests becomes unnecessarily hard

Why?

How much of this code can we move in internal/?

Should not be a big deal, but then we might need to remove all ginkgo/gomega mentions since those are supposed to be used only under test code

@ffromani
Copy link
Member

if we add in test/utils adding tests becomes unnecessarily hard

Why?

There's no inherent reason, we can fix with enough effort. But considering that utils is a bad pattern anyway, I'd rather keep moving code towards internal or pkg and/or dissolve them in the calling sites (tests) rather than adding stuff in test/utils. With some targeted exceptions of course.

How much of this code can we move in internal/?

Should not be a big deal, but then we might need to remove all ginkgo/gomega mentions since those are supposed to be used only under test code

That's fair. clients can stay there, the stuff filed under nodepools can probably be moved, at least a big chunk of it, in internal/wait (which is untested, but that's another problem for another day)

@ffromani
Copy link
Member

ok, to summarize:

  • clients: no change needed
  • hypershift: no change needed, we can check again later
  • nodepools: please move most that you can in internal/wait

unit tests added in this process are a nice bonus but not required

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

/hold cancel
#openshift/release#58789 merged

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

ok, to summarize:

  • clients: no change needed
  • hypershift: no change needed, we can check again later
  • nodepools: please move most that you can in internal/wait

unit tests added in this process are a nice bonus but not required

Done.
About unit tests - it's a bit unusual to write a unit tests for a test code.

Anyway, this code will be exercised as part of #1071, we can wait for #1071 to be completed and only if hypershift CI is passing we'll merge this one

@ffromani
Copy link
Member

Done. About unit tests - it's a bit unusual to write a unit tests for a test code.

This is exactly why we should move code in pkg or internal :)

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

Done. About unit tests - it's a bit unusual to write a unit tests for a test code.

This is exactly why we should move code in pkg or internal :)

Done

Added helper functions for interacting with nodepool.
Inspired by:
https://github.com/openshift/cluster-node-tuning-operator/blob/8aa2a81399337d2cc716c210e353b82f16645d70/test/e2e/performanceprofile/functests/utils/nodepools/nodepools.go

Those are needed for adding a `Kubeletconfig` to the hosted-cluster
where nrop tests are running.

Signed-off-by: Talor Itzhak <[email protected]>
Using the hypershift package to initialize the HyperShift MNG client
when running on HyperShift platform.

Signed-off-by: Talor Itzhak <[email protected]>
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

/override ci/prow/ci-e2e-install-hypershift

Copy link
Contributor

openshift-ci bot commented Nov 19, 2024

@Tal-or: Overrode contexts on behalf of Tal-or: ci/prow/ci-e2e-install-hypershift

In response to this:

/override ci/prow/ci-e2e-install-hypershift

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.

@Tal-or
Copy link
Collaborator Author

Tal-or commented Nov 19, 2024

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 15c8a8e into openshift-kni:main Nov 20, 2024
15 checks passed
@Tal-or Tal-or deleted the hypershfit_client branch November 20, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants