Changes required for CRD-rename StorageNode to StorageNodeSet - #36
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the simplyblock-operator Helm chart to support the CRD rename from StorageNode to StorageNodeSet, including RBAC and example manifest adjustments, and bumps the chart version.
Changes:
- Rename the StorageNode CRD to StorageNodeSet (kind/plural/metadata.name) and update bundled custom resources accordingly.
- Update RBAC rules (manager + storagenodeset roles) and controlplane ServiceAccount permissions to target
storagenodesetsresources. - Add/adjust validations and schema fields in other CRDs (e.g., StorageCluster immutability validations, BackupImport field validation).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/simplyblock-operator/templates/roles/storagenodeset_viewer_role.yaml | Updates viewer role name and resources to storagenodesets + status. |
| charts/simplyblock-operator/templates/roles/storagenodeset_editor_role.yaml | Updates editor role resources to storagenodesets + status. |
| charts/simplyblock-operator/templates/roles/storagenodeset_admin_role.yaml | Updates admin role name and resources to storagenodesets + status. |
| charts/simplyblock-operator/templates/roles/manager_role.yaml | Adjusts manager RBAC (events verbs) and switches CR permissions to storagenodesets. |
| charts/simplyblock-operator/templates/controlplane_sa.yaml | Switches controlplane SA permissions from storagenodes to storagenodesets. |
| charts/simplyblock-operator/operator_customresources.yaml | Updates example custom resource kind from StorageNode to StorageNodeSet. |
| charts/simplyblock-operator/crds/storage.simplyblock.io_storagenodesets.yaml | Renames the CRD from storagenodes to storagenodesets (kind/plural/metadata.name/descriptions). |
| charts/simplyblock-operator/crds/storage.simplyblock.io_storageclusters.yaml | Adds immutability validations and adjusts StorageCluster status schema. |
| charts/simplyblock-operator/crds/storage.simplyblock.io_controlplanes.yaml | Updates doc text to reference StorageNodeSet inheritance semantics. |
| charts/simplyblock-operator/crds/storage.simplyblock.io_backupimports.yaml | Adds a regex pattern constraint for spec.sourceBackupID. |
| charts/simplyblock-operator/Chart.yaml | Bumps chart version from 26.2.6 to 26.2.7. |
Comments suppressed due to low confidence (2)
charts/simplyblock-operator/templates/roles/storagenodeset_editor_role.yaml:18
- The ClusterRole name is still
storagenode-editor-role, which is inconsistent with the StorageNode → StorageNodeSet rename and with the otherstoragenodeset_*_role.yamlfiles. This will leave the old role name in place and can confuse role bindings/documentation.
charts/simplyblock-operator/crds/storage.simplyblock.io_storagenodesets.yaml:11 - Changing the CRD
metadata.name/plural fromstoragenodestostoragenodesetsis effectively introducing a new CRD. Helm does not upgrade CRDs in thecrds/directory for existing releases, so upgrades can end up with only the oldstoragenodesCRD present and the operator expectingstoragenodesets. Consider documenting/automating the migration (or shipping both CRDs during a transition) to avoid broken upgrades.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sourceBackupID: | ||
| description: SourceBackupID is the UUID of the backup on the source | ||
| cluster to import. | ||
| pattern: ^[a-zA-Z0-9_-]{1,128}$ | ||
| type: string |
There was a problem hiding this comment.
Is this intentional? Then the name should be adapted. Otherwise the Regex should be narrowed.
There was a problem hiding this comment.
That the regex used here is less restrictive than the name indicates. The field seems to require an UUID to be passed, it should enforce that via the regex.
There was a problem hiding this comment.
This needs to be fixed in the source (simplyblock-operator) and will be sync'd to this repository. I'll create the PR on the operator repository.
mxsrc
left a comment
mxsrc
left a comment
There was a problem hiding this comment.
Looks largely good, though some of it seems unrelated.
| sourceBackupID: | ||
| description: SourceBackupID is the UUID of the backup on the source | ||
| cluster to import. | ||
| pattern: ^[a-zA-Z0-9_-]{1,128}$ | ||
| type: string |
There was a problem hiding this comment.
Is this intentional? Then the name should be adapted. Otherwise the Regex should be narrowed.
Some seemed unrelated as he did a CRD sync with what we currently have in operator repo |
|
@mxsrc as @geoffrey1330 mentioned, it is a sync with the operator repository which seems like it wasn't done in a while. |
Changes required for simplyblock/simplyblock-operator#287 (StorageNode -> StorageNodeSet rename)