✨add ClusterClaimConfiguration to API #363
Conversation
Signed-off-by: Omar Farag <omarfarag74@gmail.com>
Signed-off-by: Omar Farag <omarfarag74@gmail.com>
|
I think this should under registrationConfiguration field cc @elgnay |
|
The structure looks good to me. As @qiujian16 suggested, please move |
|
@qiujian16 may I get this reviewed? I'd like to merge it for open-cluster-management-io/ocm#933 |
Signed-off-by: Omar Farag <omarfarag74@gmail.com>
|
@qiujian16 sorry, I didn't see your comments for some reason. Updated! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: o-farag, 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 |
| // ClusterClaimConfiguration represents the configuration of ClusterClaim | ||
| // Effective only when the `ClusterClaim` feature gate is enabled. | ||
| // +optional | ||
| ClusterClaimConfiguration *ClusterClaimConfiguration `json:"clusterClaimConfiguration,omitempty"` |
There was a problem hiding this comment.
| ClusterClaimConfiguration *ClusterClaimConfiguration `json:"clusterClaimConfiguration,omitempty"` | |
| ClusterClaims *ClusterClaimConfiguration `json:"clusterClaims,omitempty"` |
nit, not sure if it is better/simpler to remove the suffix configuration and make it as clusterClaim or clusterClaims
There was a problem hiding this comment.
I'm good with the name ClusterClaims. @qiujian16 Is there any naming convention or best practice we should follow when defining the API?
There was a problem hiding this comment.
I kinda think it is a configuration, clusterClaims might be confusing on allowing user to specify claims they would like to create.
There was a problem hiding this comment.
I agree with quijian16, since this configures cluster claims and aren't actual clusterclaims
|
|
||
| // Custom suffixes for reserved ClusterClaims. | ||
| // +optional | ||
| ReservedClusterClaimSuffixes []string `json:"reservedClusterClaimSuffixes,omitempty"` |
There was a problem hiding this comment.
I do not quite understand why a customer needs to configure the reserved cluster claims?
There was a problem hiding this comment.
I do not quite understand why a customer needs to configure the reserved cluster claims?
@zhujian7 the proposed enhancement outlines this addition, I'm not sure if a specific customer requested it
There was a problem hiding this comment.
It is guaranteed that reserved ClusterClaims will be reported in the status of the ManagedCluster. However, custom ClusterClaims are not guaranteed to appear due to the limitation on the number of custom ClusterClaims, even if the user sets a relatively high limit.
There was a problem hiding this comment.
Moreover, ClusterClaims matching the ReservedClusterClaimSuffixes should be treated as additional reserved claims. They must be merged with, not override, the built-in (hardcoded) reserved claims when reported to the hub. Please either include this information in the field comment or consider renaming the field to AdditionalReservedClusterClaimSuffixes for clarity.
There was a problem hiding this comment.
We could start with a predefined suffix. I do not know if there are use cases that need is to be configurable, and I am not sure what it should be when user change the suffix from one to another.
Summary
Enhances API to allow support for custom ClusterClaim configurations.
Related issue(s)
Required for open-cluster-management-io/ocm#897