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

CNF-15754: nodegroups refactor #1142

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Jan 7, 2025

Moving validation and tree fetching logic into helper functions which are per platform use.

Fixes: #1086 and #1085

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 7, 2025
@openshift-ci openshift-ci bot requested review from mrniranjan and shajmakh January 7, 2025 08:47
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

[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 Jan 7, 2025
@Tal-or Tal-or force-pushed the nodegroups_refactor branch from 9c0890f to 6d0b0c3 Compare January 7, 2025 10:56
@Tal-or Tal-or changed the title WIP: nodegroups refactor nodegroups refactor Jan 7, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 7, 2025
@Tal-or Tal-or changed the title nodegroups refactor CNF-15754: nodegroups refactor Jan 7, 2025
@openshift-ci-robot
Copy link

@Tal-or: This pull request references CNF-15754 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Moving validation and tree fetching logic into helper functions with are per platform.

Fixes: #1086 and #1085

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-robot
Copy link

@Tal-or: This pull request references CNF-15754 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Moving validation and tree fetching logic into helper functions which are per platform use.

Fixes: #1086 and #1085

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.

@Tal-or Tal-or force-pushed the nodegroups_refactor branch from 6d0b0c3 to a34236e Compare January 7, 2025 12:31
Tal-or added 10 commits January 7, 2025 14:33
A common interface for managing nodegroups.
This interface will be inplemented for OpenShift and HyperShift
platforms.

Signed-off-by: Talor Itzhak <[email protected]>
Add common validation functions that will be used
for both implementations of HyperShift and OpenShift.

Signed-off-by: Talor Itzhak <[email protected]>
Implement the `Manager` infterface for OpenShift.
The functionality is copied from validation package,
with slight changes in the functions names and signatures,
so no intended changes in the behavior.

Signed-off-by: Talor Itzhak <[email protected]>
Implement the `Manager` infterface for HyperShift.
The functionality is copied from validation package,
with slight changes in the functions names and signatures,
so no intended changes in the behavior.

Signed-off-by: Talor Itzhak <[email protected]>
Consume the manager as part of the reconciler struct.
In future commit we'll use the actual methods
provided by the manager.

Signed-off-by: Talor Itzhak <[email protected]>
replace the existing functions with the methods provided
by the manager.
No intended changes in the behavior.

Signed-off-by: Talor Itzhak <[email protected]>
Now that we have `nodegroups` package it seems
more appropriate to have a specific error
for validation of nodegroups under a package with the same name.

Signed-off-by: Talor Itzhak <[email protected]>
In hypershift case it will exit the functions doing nothing
becuase the mcp slice it empty.

In case that the user does provide an mcp selector
on hypershift, we'll have a validation that will report about that.

So we won't reach this code flow anyway.

Signed-off-by: Talor Itzhak <[email protected]>
Signed-off-by: Talor Itzhak <[email protected]>
Now that each platform is implementing it own validation,
we can remove the old validation which used to branch per
each platform.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or Tal-or force-pushed the nodegroups_refactor branch from a34236e to 45a3060 Compare January 7, 2025 12:33
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 8, 2025

/cc @ffromani

@openshift-ci openshift-ci bot requested a review from ffromani January 8, 2025 14:42
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 12, 2025

/cc @ffromani PTAL this is a perquisite for the abstraction of the e2e serial suite so it will support HyperShift

Copy link
Contributor

openshift-ci bot commented Jan 12, 2025

@Tal-or: GitHub didn't allow me to request PR reviews from the following users: a, e2e, PTAL, abstraction, it, will, HyperShift, this, of, suite, so, serial, is, perquisite, for, the.

Note that only openshift-kni members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ffromani PTAL this is a perquisite for the abstraction of the e2e serial suite so it will support 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.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redesign the hypershift/openshift validation logic
2 participants