OCPBUGS-65819: fix ClusterMonitoring CRD plural name #2588
Conversation
|
Pipeline controller notification For optional jobs, comment |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
WalkthroughRenames the ClusterMonitoring API resource plural from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/XXL
danielmellado
force-pushed
the
fix-clustermonitoring-plural
branch
from
669b712 to
b307806
Compare
openshift-ci
bot
added
size/M
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
config/v1alpha1/types_cluster_monitoring.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(2 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml(1 hunks)openapi/openapi.json(5 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- openapi/openapi.json
🚧 Files skipped from review as they are similar to previous changes (2)
- config/v1alpha1/types_cluster_monitoring.go
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
🔇 Additional comments (5)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml (1)
11-11: LGTM; matches the corrected GVR. Please also confirm no stale singular references remain.Same upgrade consistency checks apply as in the TechPreview CRD comment. If helpful, reuse the provided repo scan script to ensure there’s no leftover clustermonitoring.* or plural: clustermonitoring.
Also applies to: 17-17
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml (1)
11-11: Consistent, correct pluralization; finalize by confirming upgrade path.Approve the change. Please ensure:
- Old CRD name is removed from payloads to prevent duplicate CRD registration.
- Any RBAC or client usage in downstream operators references resource “clustermonitorings”.
I can help prep a checklist/notes if needed.
Also applies to: 17-17
config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
48-65: Rename migration verified as complete.Verification confirms the old singular
clustermonitoring.config.openshift.ioentry is not present in the manifest. The rename toclustermonitorings.config.openshift.io(plural) is complete, and related fields (CRDName, PluralName) are correctly updated. No duplicate registration risk.config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
11-11: CRD integration verified across aggregator and consumers.Verification confirms the CRD is properly integrated: the aggregator file correctly references
clustermonitorings.config.openshift.io(lines 48, 52), and the plural resourceclustermonitoringsis consistently used across all payload manifests, the aggregator index, and the kubebuilder annotation. No issues found.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml (1)
11-11: Generated manifest pluralization verified across all CRD variants.Verification confirms:
- All 3 CRD variants (Custom, Dev, Tech preview) present in both config/v1alpha1 and payload-manifests
- metadata.name identical:
clustermonitorings.config.openshift.io(line 11)- spec.names.plural identical:
clustermonitorings(line 17)- No singular artifacts remain
...ed.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| release.openshift.io/feature-set: TechPreviewNoUpgrade | ||
| name: clustermonitoring.config.openshift.io | ||
| name: clustermonitorings.config.openshift.io |
There was a problem hiding this comment.
🧩 Analysis chain
Correct pluralization fix; verify cleanup of the old CRD and references.
Change looks right. Please ensure the previous CRD name/plural aren’t lingering anywhere (payload, generated, RBAC, controllers) to avoid dueling CRDs or broken watches.
Run from repo root:
If the singular-named CRD ever shipped, add an upgrade note and a one-time manifest to delete clustermonitoring.config.openshift.io or a migration playbook.
Also applies to: 17-17
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Leftovers of old CRD name/plural/path"
rg -nP 'clustermonitoring\.config\.openshift\.io\b|plural:\s*clustermonitoring\b|kubebuilder:resource.*\bpath=clustermonitoring\b' || true
echo "2) Any payload CRD files still using singular in filename"
fd -a -p '0000_10_config-operator_01_*clustermonitoring*.crd.yaml' payload-manifests config/v1alpha1 || true
echo "3) Kubebuilder marker now pluralized"
rg -nP 'kubebuilder:resource.*\bpath=clustermonitorings\b' config/v1alpha1/types_cluster_monitoring.go
echo "4) RBAC rules use new resource name"
rg -nP '\bresources:\s*(\[[^\]]*\bclustermonitorings\b[^\]]*\]|.*\bclustermonitorings\b)' -g '!**/vendor/**' -t yamlLength of output: 503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check test file content and purpose ==="
cat config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml | head -10
echo ""
echo "=== Look for ClusterRole/Role RBAC files ==="
fd -t f '\.(yaml|yml)$' -x grep -l 'kind:\s*\(ClusterRole\|Role\)' {} \; | head -20
echo ""
echo "=== Search for RBAC rules referencing 'clustermonitoring' or 'clustermonitorings' ==="
rg -nP 'resources:.*\bclustermonitoring' -t yaml -g '!**/vendor/**' -g '!**/test/**' || echo "No direct RBAC references found"
echo ""
echo "=== Check if there are other test fixture files with similar structure ==="
fd -t f 'config/v1alpha1/tests/' -x grep -l 'crdName:' {} \; | head -10Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for any file containing 'crdName: clustermonitoring' ==="
rg -l 'crdName:\s*clustermonitoring\.config\.openshift\.io' -t yaml || echo "No matches found"
echo ""
echo "=== List all yaml files in config/v1alpha1/tests/ if it exists ==="
if [ -d "config/v1alpha1/tests" ]; then
find config/v1alpha1/tests -type f -name "*.yaml" -o -name "*.yml" | head -20
else
echo "Directory config/v1alpha1/tests does not exist"
fi
echo ""
echo "=== Search for clustermonitoring in config directory broadly ==="
rg 'crdName.*clustermonitoring' -t yaml config/ --max-count=5 || echo "No crdName matches in config/"
echo ""
echo "=== Check payload-manifests CRD file ==="
fd 'clustermonitorings.*\.crd\.yaml' payload-manifests -x cat {} \; | head -30Length of output: 2649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of AAA_ungated.yaml ==="
cat config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml
echo ""
echo "=== Content of ClusterMonitoringConfig.yaml ==="
cat config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
echo ""
echo "=== Check if these test files have a pattern across other CRDs ==="
rg -A2 'crdName:' config/v1alpha1/tests/ -t yaml | head -30Length of output: 16756
Update test fixture files with correct (plural) CRD name.
The pluralization fix in the payload CRD is correct, but two validation test fixture files still reference the old singular CRD name. Verify and update:
config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml: changecrdName: clustermonitoring.config.openshift.iotocrdName: clustermonitorings.config.openshift.ioconfig/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml: changecrdName: clustermonitoring.config.openshift.iotocrdName: clustermonitorings.config.openshift.io
All other CRDs in config/v1alpha1/tests/ correctly use the plural form in their test fixtures. This inconsistency may cause the validation tests to fail or use the wrong CRD schema.
🤖 Prompt for AI Agents
In
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml
around line 11 and the related test fixtures, update the two test fixture files
that still reference the old singular CRD name: open
config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml
and
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
and change the crdName value from "clustermonitoring.config.openshift.io" to the
correct plural "clustermonitorings.config.openshift.io" so they match the CRD
plural name used in the payload.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jan--f, simonpasquier 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 |
the ClusterMonitoring CRD kubebuilder annotation incorrectly specified path=clustermonitoring (singular) instead of the conventional plural form clustermonitorings. this caused a mismatch with client-go generated code which expects the plural form for list/watch operations, resulting in 'resource not found' errors when the controller attempted to watch ClusterMonitoring resources. fixes the kubebuilder:resource annotation to use the correct plural path=clustermonitorings. Signed-off-by: Daniel Mellado <[email protected]>
danielmellado
force-pushed
the
fix-clustermonitoring-plural
branch
from
b307806 to
975f565
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml (1)
11-18: DevPreviewNoUpgrade CRD rename mirrors CustomNoUpgrade manifestThis manifest carries the same CRD name/plural rename as the CustomNoUpgrade variant; the consistency is good, and the same upgrade/cleanup considerations noted there apply here as well.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml (1)
11-18: CRD rename is consistent; please confirm upgrade/cleanup behaviorThe
metadata.nameandspec.names.pluralupdates toclustermonitorings.config.openshift.io/clustermonitoringsare consistent with the Go type and other manifests. Since this effectively renames the CRD, please double-check with release/upgrade owners whether any existing clusters could already haveclustermonitoring.config.openshift.ioinstalled and, if so, whether an explicit cleanup or migration of the old CRD/CRs is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml(1 hunks)config/v1alpha1/types_cluster_monitoring.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(2 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml(1 hunks)openapi/openapi.json(5 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yamlopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-CustomNoUpgrade.crd.yaml
🔇 Additional comments (5)
openapi/openapi.json (1)
4783-4789: Auto-generated OpenAPI schema updates appear consistent and well-structured.The changes across all five hunks add comprehensive documentation and explicit enum validation for the
dnsRecordsTypefield. The description clearly enumerates allowed values (Internal,External, omitted) with detailed behavior explanations, and theenumarray properly constrains the values. The descriptions and enum values are consistently aligned.However, these changes appear to be auto-generated from source annotations (likely Go struct tags or Kubebuilder markers in the actual API types). Verify that:
- This
openapi/openapi.jsonis regenerated from source and not manually edited.- The underlying source (likely in
config/v1alpha1/types.goor similar) has been updated to include these documentation enhancements.The PR description mentions fixing ClusterMonitoring CRD plural naming, but the visible hunks only show
dnsRecordsTypeschema updates—ensure the broader PR changes (if any to ClusterMonitoring itself) are reflected correctly in the schema.Also applies to: 8936-8942, 9554-9560, 9752-9758, 11671-11677
config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
11-18: Feature-gated CRD name and plural are consistent with the Go type
metadata.nameandspec.names.pluralnow useclustermonitorings, matching the kubebuilder marker and keeping the feature-gated CRD aligned with the runtime resource path.config/v1alpha1/tests/clustermonitorings.config.openshift.io/AAA_ungated.yaml (1)
3-3: Ungated test now references the correct CRD nameUpdating
crdNametoclustermonitorings.config.openshift.iokeeps this test aligned with the renamed ClusterMonitoring CRD.config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
3-3: Feature-gated test CRD reference updated to new identifier
crdNamenow matchesclustermonitorings.config.openshift.io, keeping these validation tests in sync with the CRD rename.config/v1alpha1/types_cluster_monitoring.go (1)
35-37: Pluralized kubebuilder resource path aligns with CRD definitionChanging the kubebuilder resource marker to
path=clustermonitoringscorrectly aligns with the CRDnames.pluralfield. Verification confirms the expected structure: CRD manifests properly define both the singular form and plural form, with the plural used for resource API paths. This change resolves the client-go resource path mismatch without introducing any unintended references or breaking changes.
|
/retitle OCPBUGS-65819: fix ClusterMonitoring CRD plural name |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@danielmellado: This pull request references Jira Issue OCPBUGS-65819, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@everettraven @JoelSpeed not sure if the CI got stuck or if you'd need to somehow |
4 participants
the ClusterMonitoring CRD kubebuilder annotation incorrectly
specified path=clustermonitoring (singular) instead of the
conventional plural form clustermonitorings.
this caused a mismatch with client-go generated code which
expects the plural form for list/watch operations, resulting
in 'resource not found' errors when the controller attempted
to watch ClusterMonitoring resources.
fixes the kubebuilder:resource annotation to use the correct
plural path=clustermonitorings.
Signed-off-by: Daniel Mellado [email protected]