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

proposal change for clusterset api #48

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ldpliu
Copy link
Contributor

@ldpliu ldpliu commented Mar 29, 2022

Signed-off-by: ldpliu [email protected]

@ldpliu
Copy link
Contributor Author

ldpliu commented Mar 29, 2022

/cc @qiujian16 @elgnay

@openshift-ci openshift-ci bot requested a review from elgnay March 29, 2022 06:16
@elgnay
Copy link
Contributor

elgnay commented Apr 13, 2022

/lgtm

// ManagedClusterSelector represents a selector of ManagedClusters
type ManagedClusterSelector struct {
// SelectorType could only be "LegacyClusterSetLabel" now, will support more SelectorType later
// "LegacyClusterSetLabel" means to use label "cluster.open-cluster-management.io/clusterset:<ManagedClusterSet Name>"" to select target clusters.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add labelSelector

```

- `LabelSelector` will not be included
Copy link
Member

Choose a reason for hiding this comment

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

let's keep the labelSelector since it should done in this release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

b. `submariner-addon` uses managedClusterSet group clusters based on the network. And in different managedClusterSet, the clusters should be exclusive. So it should only watch the following managedClusterSet:
- `spec.ClusterSelector.SelectorType` is `LegacyClusterSetLabel`

c. `placement` using new `ClusterSelector` to select target clusters.

3. [Implement in OCM 0.8.0] Update full managedClusterSet api and RBAC
- Include `LabelSelector`
Copy link
Member

Choose a reason for hiding this comment

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

I think exclusiveKey is not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think exclusiveKey is not necessary?

I am not really understand what you mean, Do you mean we should remove the ExclusiveLabel in selectorType ?

@openshift-ci
Copy link

openshift-ci bot commented Apr 13, 2022

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Aug 9, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ldpliu
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval by writing /assign @deads2k in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants