OCPBUGS-77146: Remove status field from CatalogSource, ClusterCatalog…#1369
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@dorzel: This pull request references Jira Issue OCPBUGS-77146, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRemoves the top-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dorzel: This pull request references Jira Issue OCPBUGS-77146, 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: DetailsIn 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. |
|
@dorzel: This pull request references Jira Issue OCPBUGS-77146, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/pkg/clusterresources/clusterresources.go (1)
639-646: Prefer structural sanitization forUpdateServicetoo.This replacement is tied to the current YAML emitter output and only removes the empty-object form. If
UpdateServiceStatusever stops serializing as exactlystatus: {}, the field will leak back into the generated manifest. Using the same unstructured-delete flow as the other generators would make this path much harder to regress.♻️ Suggested direction
- // put UpdateService in yaml - osusBytes, err := yaml.Marshal(osus) + unstructuredObj := unstructured.Unstructured{} + unstructuredObj.Object, err = runtime.DefaultUnstructuredConverter.ToUnstructured(&osus) + if err != nil { + return err + } + delete(unstructuredObj.Object["metadata"].(map[string]interface{}), "creationTimestamp") + delete(unstructuredObj.Object, "status") + + // put UpdateService in yaml + osusBytes, err := yaml.Marshal(unstructuredObj.Object) if err != nil { return err } - // creationTimestamp is a struct, omitempty does not apply - osusBytes = bytes.ReplaceAll(osusBytes, []byte(" creationTimestamp: null\n"), []byte("")) - osusBytes = bytes.ReplaceAll(osusBytes, []byte("status: {}\n"), []byte(""))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/clusterresources/clusterresources.go` around lines 639 - 646, The current code uses bytes.ReplaceAll on yaml.Marshal output for osus to strip "creationTimestamp: null" and "status: {}" which is fragile; instead perform structural sanitization like the other generators: convert the UpdateService object (osus) into an unstructured map (or decode osusBytes into a map[string]interface{}), remove the "status" key and any metadata.creationTimestamp entries from that map, then re-marshal that cleaned map into YAML (replacing the current use of bytes.ReplaceAll on osusBytes). Target symbols: osus (the UpdateService object), osusBytes, and the UpdateService/UpdateServiceStatus fields to locate where to replace the string-replace logic with an unstructured delete flow.internal/pkg/clusterresources/clusterresources_test.go (1)
743-745: Please cover the template-backedCatalogSourcepath too.This new assertion only exercises the default builder. Since
TargetCatalogSourceTemplateaccepts user-provided YAML, that subtest is the likeliest place for a top-levelstatusblock to sneak back in. ReusingverifyNoStatusFieldthere would lock down the regression more completely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/clusterresources/clusterresources_test.go` around lines 743 - 745, Add the same verifyNoStatusField assertion to the subtest that uses TargetCatalogSourceTemplate: after the template-backed CatalogSource is rendered and its file is written (the csFiles entry produced by that subtest), call verifyNoStatusField(t, filepath.Join(workingDir, clusterResourcesDir, <that csFiles entry>.Name())) so the CatalogSource created from TargetCatalogSourceTemplate is also checked for a top-level status field; reference the existing verifyNoStatusField helper and the TargetCatalogSourceTemplate subtest’s csFiles entry to locate where to insert the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/pkg/clusterresources/clusterresources_test.go`:
- Around line 743-745: Add the same verifyNoStatusField assertion to the subtest
that uses TargetCatalogSourceTemplate: after the template-backed CatalogSource
is rendered and its file is written (the csFiles entry produced by that
subtest), call verifyNoStatusField(t, filepath.Join(workingDir,
clusterResourcesDir, <that csFiles entry>.Name())) so the CatalogSource created
from TargetCatalogSourceTemplate is also checked for a top-level status field;
reference the existing verifyNoStatusField helper and the
TargetCatalogSourceTemplate subtest’s csFiles entry to locate where to insert
the assertion.
In `@internal/pkg/clusterresources/clusterresources.go`:
- Around line 639-646: The current code uses bytes.ReplaceAll on yaml.Marshal
output for osus to strip "creationTimestamp: null" and "status: {}" which is
fragile; instead perform structural sanitization like the other generators:
convert the UpdateService object (osus) into an unstructured map (or decode
osusBytes into a map[string]interface{}), remove the "status" key and any
metadata.creationTimestamp entries from that map, then re-marshal that cleaned
map into YAML (replacing the current use of bytes.ReplaceAll on osusBytes).
Target symbols: osus (the UpdateService object), osusBytes, and the
UpdateService/UpdateServiceStatus fields to locate where to replace the
string-replace logic with an unstructured delete flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b5164d6-ee85-446d-856b-7e748e5b06c8
📒 Files selected for processing (2)
internal/pkg/clusterresources/clusterresources.gointernal/pkg/clusterresources/clusterresources_test.go
…, and UpdateService yaml files
|
/retest |
openshift-ci
Bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aguidirh, dorzel, nidangavali The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @nidangavali |
openshift-ci-robot
added
the
verified
|
@nidangavali: This PR has been marked as verified by DetailsIn 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. |
|
@r4f4 could you please have a look when you have time? |
|
/test integration |
|
@dorzel: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@dorzel: Jira Issue Verification Checks: Jira Issue OCPBUGS-77146 Jira Issue OCPBUGS-77146 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-25-221249 |
|
/cherrypick release-4.21 release-4.20 |
|
@dorzel: new pull request created: #1383 DetailsIn 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 kubernetes-sigs/prow repository. |
7 participants
…, and UpdateService yaml files
Description
This removes the unneeded status field from generated CatalogSource, ClusterCatalog, and UpdateService yaml files. Similar fix to #1311.
Github / Jira issue: https://issues.redhat.com/browse/OCPBUGS-77146
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Ran the m2d and d2m workflow described in the bug, with the ISC provided in the bug. Verified that that status field was gone on the generated CatalogSource, ClusterCatalog, and UpdateService files in the mirror working dir.
Expected Outcome
Status field no longer exists on the files in question.
Summary by CodeRabbit
Bug Fixes
Tests