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

✨Updated clustermanager API spec and generated files for spec change. #355

Conversation

dtclxy64
Copy link
Contributor

@dtclxy64 dtclxy64 commented Jan 3, 2025

Summary

Updated clustermanager API spec and generated files for spec change.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from deads2k and qiujian16 January 3, 2025 22:09

// This provides details to initialize Hub cluster
// +optional
RegistrationDriverHub RegistrationDriverHub `json:"registrationDriverHub,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

we can call it authDriver or registrationDriver. Also I think this should be a list, we should be able to support multiple drivers at the same time on hub.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated with the suggestions

Copy link
Member

Choose a reason for hiding this comment

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

could you make this as

registrationDrivers []RegistrationDriver

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have another struct with name RegistrationDriver in types_klusterlet.go. That is the reason it was initially named it as RegistrationDriverHub. So can we go with the second option that you had suggested which is authDriver?

Copy link
Member

Choose a reason for hiding this comment

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

it is ok that the struct name is RegistrationDriverHub, but let's make the field name to be registrationDrivers and make it as a slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed to registrationDrivers and it was already changed to a slice in previous commit

@@ -108,6 +108,19 @@ type RegistrationHubConfiguration struct {
// he can set featuregate/Foo=false before upgrading. Let's say the cluster-admin wants featuregate/Foo=false.
// +optional
FeatureGates []FeatureGate `json:"featureGates,omitempty"`

// This provides details to initialize Hub cluster
Copy link
Member

Choose a reason for hiding this comment

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

the comment needs to be updated, it is not clear what this drivers meant to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Modified the comment to give more clarity

@amrcoder amrcoder force-pushed the 65928-clustermanager branch from 5f71654 to 9c4a71e Compare January 6, 2025 16:54
// +required
// +kubebuilder:default:=csr
// +kubebuilder:validation:Enum=csr;awsirsa
AuthType string `json:"authType,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

one minor comment, we should add

// +listType=map
// +listMapKey=authType
here, so we will not set duplicate type

@qiujian16
Copy link
Member

/approve
Just one minor comment, feel free to merge it when it is resolved.

@openshift-ci openshift-ci bot added the approved label Jan 9, 2025
@mikeshng
Copy link
Member

mikeshng commented Jan 9, 2025

/lgtm

I've verified that the following are added:

// +listType=map
// +listMapKey=authType

Copy link
Contributor

openshift-ci bot commented Jan 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtclxy64, mikeshng, qiujian16

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-merge-bot openshift-merge-bot bot merged commit bda1321 into open-cluster-management-io:main Jan 9, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants