-
Notifications
You must be signed in to change notification settings - Fork 189
ocsinit: remove stale code from ocsinit #3553
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
base: main
Are you sure you want to change the base?
Conversation
rewantsoni
commented
Oct 15, 2025
- CSI settings are controlled by ocs-client-op via ceph-csi-op. We no longer need to specify these settings on the ocs-op configmap
- Clusters object is no longer needed in ocsinit, we just need to know the number of storageClusters to set the RookCurrentNamespaceOnlyKey var
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rewantsoni 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 |
Signed-off-by: Rewant Soni <[email protected]>
123fa76 to
1ebee71
Compare
|
/retest-required |
| util.TopologyDomainLabelsKey: r.getTopologyDomainLabelsKeyValue(), | ||
| util.EnableNFSKey: r.getEnableNFSKeyValue(), | ||
| util.EnableCephfsKey: enableCephfsVal, | ||
| util.RookCurrentNamespaceOnlyKey: strconv.FormatBool(!(len(storageClusterList.Items) > 1)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we declaring another variable storageClusterList if previously we could do r.clusters.GetStorageClusters()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating the entire cluster object, I think it's better to list the storageClusters and then use them
| crdsBeingWatched sync.Map | ||
| } | ||
|
|
||
| // SetupWithManager sets up a controller with a manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any changes in the SetupWithManager func? and any reason to move it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, changed the predicate of StorageCluster watch, no reason but I think it makes it more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either do a refactor or do a fix with refactor only if required but not both in general which results in hunting for the change.
|
@rewantsoni: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@rewantsoni THis change would need to accompany changes in the downstream rook csv or else the rook-ceph-operator csv would not succeed as its pod would be in error. |