Skip to content

MG-331: Promote the must-gather-operator from Tech Preview to GA - #378

Draft
neha037 wants to merge 1 commit into
openshift:masterfrom
neha037:MG-331
Draft

MG-331: Promote the must-gather-operator from Tech Preview to GA#378
neha037 wants to merge 1 commit into
openshift:masterfrom
neha037:MG-331

Conversation

@neha037

@neha037 neha037 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Promoted the MustGather API to the stable v1 version.
    • Continued serving the previous v1alpha1 API for compatibility.
    • Added support for trusted CA bundle injection.
    • Published the operator through the stable update channel.
  • Documentation

    • Updated MustGather examples to use the stable API version.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 29, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 29, 2026

Copy link
Copy Markdown

@neha037: This pull request references MG-331 which is a valid jira issue.

Details

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.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bf1049fb-b411-4fb8-bb26-46606eb6f187

📥 Commits

Reviewing files that changed from the base of the PR and between 501f600 and 5406645.

⛔ Files ignored due to path filters (2)
  • api/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • api/v1/zz_generated.openapi.go is excluded by !**/zz_generated*
📒 Files selected for processing (31)
  • PROJECT
  • api/v1/groupversion_info.go
  • api/v1/mustgather_types.go
  • bundle/bundle.Dockerfile
  • bundle/manifests/art.yaml
  • bundle/manifests/stable/image-references
  • bundle/manifests/stable/operator.openshift.io_mustgathers.yaml
  • bundle/manifests/stable/support-log-gather-operator.clusterserviceversion.yaml
  • bundle/manifests/stable/trusted-ca.configmap.yaml
  • bundle/manifests/support-log-gather-operator.package.yaml
  • bundle/metadata/annotations.yaml
  • config/templates/csv-template.yaml
  • config/templates/must-gather-operator-csv-template.yaml
  • controllers/mustgather/mustgather_controller.go
  • controllers/mustgather/mustgather_controller_test.go
  • controllers/mustgather/mustgather_image_test.go
  • controllers/mustgather/predicates_test.go
  • controllers/mustgather/template.go
  • controllers/mustgather/template_test.go
  • controllers/mustgather/trusted_ca_test.go
  • deploy/crds/operator.openshift.io_mustgathers.yaml
  • examples/mustgather_basic.yaml
  • examples/mustgather_full.yaml
  • examples/mustgather_non_internal_user.yaml
  • examples/mustgather_retain_resources.yaml
  • examples/mustgather_timeout.yaml
  • examples/mustgather_with_pvc_subpath.yaml
  • examples/mustgather_with_since.yaml
  • examples/mustgather_with_sincetime.yaml
  • main.go
  • test/e2e/must_gather_operator_test.go

Walkthrough

The PR introduces the MustGather v1 API, updates reconciliation and tests to use it, retains served v1alpha1 compatibility, changes v1 to the CRD storage version, publishes the operator through the stable bundle channel, and updates examples.

Changes

MustGather v1 migration

Layer / File(s) Summary
Define the v1 API contract
api/v1/*
Adds v1 group registration and MustGather resource types with validation, storage, upload, status, and scheme-registration definitions.
Publish v1 and retain v1alpha1 compatibility
PROJECT, deploy/crds/*, bundle/manifests/stable/operator.openshift.io_mustgathers.yaml, main.go
Makes v1 the stored CRD version while continuing to serve v1alpha1 and registering v1 with the manager.
Migrate controller reconciliation and job templates
controllers/mustgather/mustgather_controller.go, controllers/mustgather/template.go
Switches reconciliation, status, cleanup, image, trusted CA, storage, and upload handling to v1 types.
Update unit and integration validation
controllers/mustgather/*_test.go, test/e2e/must_gather_operator_test.go
Migrates controller and end-to-end coverage to v1 resource creation, retrieval, status assertions, cleanup, and dual-serve checks.
Publish the stable operator bundle
bundle/*, config/templates/*, examples/*
Changes bundle maturity and channels to stable, adds stable image and trusted CA manifests, and updates examples to v1.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: praveencodes

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Microshift Test Compatibility ⚠️ Warning New e2e context API Version Dual-Serve Tests is untagged and uses MustGather (operator.openshift.io) with no MicroShift skip/guard. Add [apigroup:operator.openshift.io] or [Skipped:MicroShift], or guard with exutil.IsMicroShiftCluster() and g.Skip().
✅ Passed checks (13 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the PR’s main change: promoting must-gather-operator from Tech Preview to GA.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No added/edited Ginkgo title strings in the diff, and the existing titles are static with no dynamic interpolation.
Test Structure And Quality ✅ Passed The touched Ginkgo tests use BeforeAll/AfterAll and AfterEach cleanup, and cluster waits are timeout-bounded; no clear structural regressions were introduced.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added/updated e2e specs are CRUD/PVC/SFTP/API-version checks; the only node assertion just waits for a pod NodeName and no multi-node or HA assumptions appear.
Topology-Aware Scheduling Compatibility ✅ Passed No new HA-only scheduling constraints were added; the only pod scheduling logic is preferred infra affinity plus infra toleration, not required anti-affinity or topology spread.
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes found; main uses structured logging, and suite/init code only registers schemes or uses GinkgoWriter/By.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No hardcoded IPv4 found in test/e2e; IPv6 host handling uses JoinHostPort; public SFTP cases are marked [Skipped:Disconnected].
No-Weak-Crypto ✅ Passed PASS: Changed files contain no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, no custom crypto, and no secret/token comparisons; changes are API/manifest only.
Container-Privileges ✅ Passed No touched manifest or workload template adds privileged:true, host* settings, allowPrivilegeEscalation:true, SYS_ADMIN, or root-only securityContext.
No-Sensitive-Data-In-Logs ✅ Passed No new log/print statements were added in the diff; existing logs only mention resource names/namespaces and SFTP errors, not credentials or tokens.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2026
@openshift-ci

openshift-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@neha037

neha037 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: neha037
Once this PR has been reviewed and has the lgtm label, please assign smarthall for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.94%. Comparing base (501f600) to head (5406645).

Files with missing lines Patch % Lines
controllers/mustgather/mustgather_controller.go 90.00% 1 Missing ⚠️
main.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   82.57%   81.94%   -0.63%     
==========================================
  Files           9       10       +1     
  Lines         918      925       +7     
==========================================
  Hits          758      758              
- Misses        148      155       +7     
  Partials       12       12              
Files with missing lines Coverage Δ
api/v1/mustgather_types.go 100.00% <100.00%> (ø)
controllers/mustgather/template.go 100.00% <100.00%> (ø)
controllers/mustgather/mustgather_controller.go 93.98% <90.00%> (ø)
main.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (3)
api/v1/mustgather_types.go (1)

207-214: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

patchStrategy/patchMergeKey struct tags are inert for CRDs; use listType markers.

For CRD-based APIs the merge semantics come from +listType=map / +listMapKey=type markers, which controller-gen renders as x-kubernetes-list-type in the schema — the generated YAML currently emits a plain array. Adding them also lets Server-Side Apply merge conditions per type instead of replacing the whole list.

♻️ Proposed change
-	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
+	// +listType=map
+	// +listMapKey=type
+	// +patchStrategy=merge
+	// +patchMergeKey=type
+	// +optional
+	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v1/mustgather_types.go` around lines 207 - 214, Update
MustGatherStatus.Conditions to use CRD list markers +listType=map and
+listMapKey=type, removing reliance on the inert patchStrategy and patchMergeKey
tags. Ensure controller-gen emits x-kubernetes-list-type: map with type as the
list map key so Server-Side Apply merges conditions by type.
deploy/crds/operator.openshift.io_mustgathers.yaml (1)

17-17: 🗄️ Data Integrity & Integration | 🔵 Trivial

Storage-version flip needs a storage migration before v1alpha1 can ever be dropped.

Both versions carry identical schemas, so the implicit strategy: None conversion is safe here. But existing objects stay etcd-encoded as v1alpha1, and the API server will append v1alpha1 to status.storedVersions. Keeping served: true (as done) is correct; plan a storage migration (e.g. a StorageVersionMigration or a read/write pass over all MustGathers) plus a storedVersions prune before any future removal of v1alpha1, otherwise the CRD update will be rejected.

Also applies to: 319-323, 625-628

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deploy/crds/operator.openshift.io_mustgathers.yaml` at line 17, Document and
plan a storage migration for the MustGather CRD before removing v1alpha1:
migrate existing objects to v1, then prune v1alpha1 from status.storedVersions.
Keep v1alpha1 served during the migration and preserve the existing implicit
None conversion behavior.
bundle/manifests/stable/operator.openshift.io_mustgathers.yaml (1)

17-17: 🗄️ Data Integrity & Integration | 🔵 Trivial

Same storage-version migration consideration as the deploy/crds copy. This is the manifest OLM applies on upgrade from the Tech Preview bundle, so it is the one that will populate status.storedVersions with both versions on existing clusters.

Also applies to: 625-628

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bundle/manifests/stable/operator.openshift.io_mustgathers.yaml` at line 17,
Apply the same storage-version migration changes used in the deploy/crds copy to
this OLM-applied manifest’s v1 version entry. Ensure upgrades from the Tech
Preview bundle preserve both API versions in status.storedVersions and correctly
transition the existing cluster’s storage version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bundle/manifests/stable/image-references`:
- Around line 2-4: Add the required metadata.name field to the ImageStream
manifest alongside kind and apiVersion, using the expected stable ImageStream
name; leave the existing spec unchanged.

In
`@bundle/manifests/stable/support-log-gather-operator.clusterserviceversion.yaml`:
- Line 59: Update the stable ClusterServiceVersion’s operator and must-gather
image references to immutable version tags or digests, replacing every mutable
:latest tag while preserving the existing GA image fields and structure.

In `@controllers/mustgather/mustgather_controller_test.go`:
- Line 13: Update the mustgatherv1 import usage in the must-gather controller
test so it matches the corrected api/v1 package definition and compiles
successfully. Inspect the referenced API types and adjust the import or affected
symbols consistently without changing the test’s behavior.

In `@controllers/mustgather/mustgather_image_test.go`:
- Line 10: Update the mustgatherv1 import usage in the test file to resolve the
api/v1 compilation failure, ensuring all references in the test remain
consistent with the corrected API package or symbols.

In `@controllers/mustgather/predicates_test.go`:
- Line 6: Update the mustgatherv1 import usage in the predicates tests to
resolve the api/v1 compilation failure, ensuring all references remain
consistent with the corrected API package symbols.

In `@controllers/mustgather/template_test.go`:
- Line 13: Update the mustgatherv1 import usage in the test file to resolve the
api/v1 compilation failure identified by static analysis. Ensure all references
in the test remain consistent with the corrected API package or symbols, without
changing unrelated test behavior.

In `@controllers/mustgather/template.go`:
- Line 16: Update the mustgatherv1 API types used by
controllers/mustgather/template.go so the imported api/v1 package compiles,
including implementing the required DeepCopyObject method on the affected
runtime.Object type. Preserve the existing mustgatherv1 import and controller
behavior.

In `@controllers/mustgather/trusted_ca_test.go`:
- Line 9: Update the mustgatherv1 import usage in trusted_ca_test.go to resolve
the api/v1 compilation failure identified by static analysis. Ensure all
references in this test use a currently valid, buildable API package or symbol
while preserving the test’s existing behavior.

---

Nitpick comments:
In `@api/v1/mustgather_types.go`:
- Around line 207-214: Update MustGatherStatus.Conditions to use CRD list
markers +listType=map and +listMapKey=type, removing reliance on the inert
patchStrategy and patchMergeKey tags. Ensure controller-gen emits
x-kubernetes-list-type: map with type as the list map key so Server-Side Apply
merges conditions by type.

In `@bundle/manifests/stable/operator.openshift.io_mustgathers.yaml`:
- Line 17: Apply the same storage-version migration changes used in the
deploy/crds copy to this OLM-applied manifest’s v1 version entry. Ensure
upgrades from the Tech Preview bundle preserve both API versions in
status.storedVersions and correctly transition the existing cluster’s storage
version.

In `@deploy/crds/operator.openshift.io_mustgathers.yaml`:
- Line 17: Document and plan a storage migration for the MustGather CRD before
removing v1alpha1: migrate existing objects to v1, then prune v1alpha1 from
status.storedVersions. Keep v1alpha1 served during the migration and preserve
the existing implicit None conversion behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bf1049fb-b411-4fb8-bb26-46606eb6f187

📥 Commits

Reviewing files that changed from the base of the PR and between 501f600 and 5406645.

⛔ Files ignored due to path filters (2)
  • api/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • api/v1/zz_generated.openapi.go is excluded by !**/zz_generated*
📒 Files selected for processing (31)
  • PROJECT
  • api/v1/groupversion_info.go
  • api/v1/mustgather_types.go
  • bundle/bundle.Dockerfile
  • bundle/manifests/art.yaml
  • bundle/manifests/stable/image-references
  • bundle/manifests/stable/operator.openshift.io_mustgathers.yaml
  • bundle/manifests/stable/support-log-gather-operator.clusterserviceversion.yaml
  • bundle/manifests/stable/trusted-ca.configmap.yaml
  • bundle/manifests/support-log-gather-operator.package.yaml
  • bundle/metadata/annotations.yaml
  • config/templates/csv-template.yaml
  • config/templates/must-gather-operator-csv-template.yaml
  • controllers/mustgather/mustgather_controller.go
  • controllers/mustgather/mustgather_controller_test.go
  • controllers/mustgather/mustgather_image_test.go
  • controllers/mustgather/predicates_test.go
  • controllers/mustgather/template.go
  • controllers/mustgather/template_test.go
  • controllers/mustgather/trusted_ca_test.go
  • deploy/crds/operator.openshift.io_mustgathers.yaml
  • examples/mustgather_basic.yaml
  • examples/mustgather_full.yaml
  • examples/mustgather_non_internal_user.yaml
  • examples/mustgather_retain_resources.yaml
  • examples/mustgather_timeout.yaml
  • examples/mustgather_with_pvc_subpath.yaml
  • examples/mustgather_with_since.yaml
  • examples/mustgather_with_sincetime.yaml
  • main.go
  • test/e2e/must_gather_operator_test.go

Comment thread controllers/mustgather/mustgather_controller_test.go

imagev1 "github.com/openshift/api/image/v1"
mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
mustgatherv1 "github.com/openshift/must-gather-operator/api/v1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Same api/v1 build-blocking issue applies here.

This test file imports mustgatherv1, which currently fails to compile per static analysis. See consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mustgather/mustgather_image_test.go` at line 10, Update the
mustgatherv1 import usage in the test file to resolve the api/v1 compilation
failure, ensuring all references in the test remain consistent with the
corrected API package or symbols.

Source: Linters/SAST tools

"testing"

mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
mustgatherv1 "github.com/openshift/must-gather-operator/api/v1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Same api/v1 build-blocking issue applies here.

This test file imports mustgatherv1, which currently fails to compile per static analysis. See consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mustgather/predicates_test.go` at line 6, Update the mustgatherv1
import usage in the predicates tests to resolve the api/v1 compilation failure,
ensuring all references remain consistent with the corrected API package
symbols.

Source: Linters/SAST tools

"time"

mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
mustgatherv1 "github.com/openshift/must-gather-operator/api/v1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Same api/v1 build-blocking issue applies here.

This test file imports mustgatherv1, which currently fails to compile per static analysis. See consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mustgather/template_test.go` at line 13, Update the mustgatherv1
import usage in the test file to resolve the api/v1 compilation failure
identified by static analysis. Ensure all references in the test remain
consistent with the corrected API package or symbols, without changing unrelated
test behavior.

Source: Linters/SAST tools

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/openshift/must-gather-operator/api/v1alpha1"
mustgatherv1 "github.com/openshift/must-gather-operator/api/v1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Same api/v1 build-blocking issue applies here.

This file imports mustgatherv1, which currently fails to compile per static analysis (missing DeepCopyObject). See consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mustgather/template.go` at line 16, Update the mustgatherv1 API
types used by controllers/mustgather/template.go so the imported api/v1 package
compiles, including implementing the required DeepCopyObject method on the
affected runtime.Object type. Preserve the existing mustgatherv1 import and
controller behavior.

Source: Linters/SAST tools

"testing"

mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
mustgatherv1 "github.com/openshift/must-gather-operator/api/v1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Same api/v1 build-blocking issue applies here.

This test file imports mustgatherv1, which currently fails to compile per static analysis. See consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mustgather/trusted_ca_test.go` at line 9, Update the mustgatherv1
import usage in trusted_ca_test.go to resolve the api/v1 compilation failure
identified by static analysis. Ensure all references in this test use a
currently valid, buildable API package or symbol while preserving the test’s
existing behavior.

Source: Linters/SAST tools

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 8

🧹 Nitpick comments (3)
api/v1/mustgather_types.go (1)

207-214: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

patchStrategy/patchMergeKey struct tags are inert for CRDs; use listType markers.

For CRD-based APIs the merge semantics come from +listType=map / +listMapKey=type markers, which controller-gen renders as x-kubernetes-list-type in the schema — the generated YAML currently emits a plain array. Adding them also lets Server-Side Apply merge conditions per type instead of replacing the whole list.

♻️ Proposed change
-	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
+	// +listType=map
+	// +listMapKey=type
+	// +patchStrategy=merge
+	// +patchMergeKey=type
+	// +optional
+	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v1/mustgather_types.go` around lines 207 - 214, Update
MustGatherStatus.Conditions to use CRD list markers +listType=map and
+listMapKey=type, removing reliance on the inert patchStrategy and patchMergeKey
tags. Ensure controller-gen emits x-kubernetes-list-type: map with type as the
list map key so Server-Side Apply merges conditions by type.
deploy/crds/operator.openshift.io_mustgathers.yaml (1)

17-17: 🗄️ Data Integrity & Integration | 🔵 Trivial

Storage-version flip needs a storage migration before v1alpha1 can ever be dropped.

Both versions carry identical schemas, so the implicit strategy: None conversion is safe here. But existing objects stay etcd-encoded as v1alpha1, and the API server will append v1alpha1 to status.storedVersions. Keeping served: true (as done) is correct; plan a storage migration (e.g. a StorageVersionMigration or a read/write pass over all MustGathers) plus a storedVersions prune before any future removal of v1alpha1, otherwise the CRD update will be rejected.

Also applies to: 319-323, 625-628

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deploy/crds/operator.openshift.io_mustgathers.yaml` at line 17, Document and
plan a storage migration for the MustGather CRD before removing v1alpha1:
migrate existing objects to v1, then prune v1alpha1 from status.storedVersions.
Keep v1alpha1 served during the migration and preserve the existing implicit
None conversion behavior.
bundle/manifests/stable/operator.openshift.io_mustgathers.yaml (1)

17-17: 🗄️ Data Integrity & Integration | 🔵 Trivial

Same storage-version migration consideration as the deploy/crds copy. This is the manifest OLM applies on upgrade from the Tech Preview bundle, so it is the one that will populate status.storedVersions with both versions on existing clusters.

Also applies to: 625-628

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bundle/manifests/stable/operator.openshift.io_mustgathers.yaml` at line 17,
Apply the same storage-version migration changes used in the deploy/crds copy to
this OLM-applied manifest’s v1 version entry. Ensure upgrades from the Tech
Preview bundle preserve both API versions in status.storedVersions and correctly
transition the existing cluster’s storage version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bundle/manifests/stable/image-references`:
- Around line 2-4: Add the required metadata.name field to the ImageStream
manifest alongside kind and apiVersion, using the expected stable ImageStream
name; leave the existing spec unchanged.

In
`@bundle/manifests/stable/support-log-gather-operator.clusterserviceversion.yaml`:
- Line 59: Update the stable ClusterServiceVersion’s operator and must-gather
image references to immutable version tags or digests, replacing every mutable
:latest tag while preserving the existing GA image fields and structure.

In `@controllers/mustgather/mustgather_controller_test.go`:
- Line 13: Update the mustgatherv1 import usage in the must-gather controller
test so it matches the corrected api/v1 package definition and compiles
successfully. Inspect the referenced API types and adjust the import or affected
symbols consistently without changing the test’s behavior.

In `@controllers/mustgather/mustgather_image_test.go`:
- Line 10: Update the mustgatherv1 import usage in the test file to resolve the
api/v1 compilation failure, ensuring all references in the test remain
consistent with the corrected API package or symbols.

In `@controllers/mustgather/predicates_test.go`:
- Line 6: Update the mustgatherv1 import usage in the predicates tests to
resolve the api/v1 compilation failure, ensuring all references remain
consistent with the corrected API package symbols.

In `@controllers/mustgather/template_test.go`:
- Line 13: Update the mustgatherv1 import usage in the test file to resolve the
api/v1 compilation failure identified by static analysis. Ensure all references
in the test remain consistent with the corrected API package or symbols, without
changing unrelated test behavior.

In `@controllers/mustgather/template.go`:
- Line 16: Update the mustgatherv1 API types used by
controllers/mustgather/template.go so the imported api/v1 package compiles,
including implementing the required DeepCopyObject method on the affected
runtime.Object type. Preserve the existing mustgatherv1 import and controller
behavior.

In `@controllers/mustgather/trusted_ca_test.go`:
- Line 9: Update the mustgatherv1 import usage in trusted_ca_test.go to resolve
the api/v1 compilation failure identified by static analysis. Ensure all
references in this test use a currently valid, buildable API package or symbol
while preserving the test’s existing behavior.

---

Nitpick comments:
In `@api/v1/mustgather_types.go`:
- Around line 207-214: Update MustGatherStatus.Conditions to use CRD list
markers +listType=map and +listMapKey=type, removing reliance on the inert
patchStrategy and patchMergeKey tags. Ensure controller-gen emits
x-kubernetes-list-type: map with type as the list map key so Server-Side Apply
merges conditions by type.

In `@bundle/manifests/stable/operator.openshift.io_mustgathers.yaml`:
- Line 17: Apply the same storage-version migration changes used in the
deploy/crds copy to this OLM-applied manifest’s v1 version entry. Ensure
upgrades from the Tech Preview bundle preserve both API versions in
status.storedVersions and correctly transition the existing cluster’s storage
version.

In `@deploy/crds/operator.openshift.io_mustgathers.yaml`:
- Line 17: Document and plan a storage migration for the MustGather CRD before
removing v1alpha1: migrate existing objects to v1, then prune v1alpha1 from
status.storedVersions. Keep v1alpha1 served during the migration and preserve
the existing implicit None conversion behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bf1049fb-b411-4fb8-bb26-46606eb6f187

📥 Commits

Reviewing files that changed from the base of the PR and between 501f600 and 5406645.

⛔ Files ignored due to path filters (2)
  • api/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • api/v1/zz_generated.openapi.go is excluded by !**/zz_generated*
📒 Files selected for processing (31)
  • PROJECT
  • api/v1/groupversion_info.go
  • api/v1/mustgather_types.go
  • bundle/bundle.Dockerfile
  • bundle/manifests/art.yaml
  • bundle/manifests/stable/image-references
  • bundle/manifests/stable/operator.openshift.io_mustgathers.yaml
  • bundle/manifests/stable/support-log-gather-operator.clusterserviceversion.yaml
  • bundle/manifests/stable/trusted-ca.configmap.yaml
  • bundle/manifests/support-log-gather-operator.package.yaml
  • bundle/metadata/annotations.yaml
  • config/templates/csv-template.yaml
  • config/templates/must-gather-operator-csv-template.yaml
  • controllers/mustgather/mustgather_controller.go
  • controllers/mustgather/mustgather_controller_test.go
  • controllers/mustgather/mustgather_image_test.go
  • controllers/mustgather/predicates_test.go
  • controllers/mustgather/template.go
  • controllers/mustgather/template_test.go
  • controllers/mustgather/trusted_ca_test.go
  • deploy/crds/operator.openshift.io_mustgathers.yaml
  • examples/mustgather_basic.yaml
  • examples/mustgather_full.yaml
  • examples/mustgather_non_internal_user.yaml
  • examples/mustgather_retain_resources.yaml
  • examples/mustgather_timeout.yaml
  • examples/mustgather_with_pvc_subpath.yaml
  • examples/mustgather_with_since.yaml
  • examples/mustgather_with_sincetime.yaml
  • main.go
  • test/e2e/must_gather_operator_test.go
🛑 Comments failed to post (1)
bundle/manifests/stable/image-references (1)

2-4: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add required ImageStream metadata.

This ImageStream lacks metadata.name (or generateName), so the manifest is invalid for creation. Add the expected stable ImageStream name before publishing the bundle.

Proposed fix
 kind: ImageStream
 apiVersion: image.openshift.io/v1
+metadata:
+  name: support-log-gather-operator
 spec:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

kind: ImageStream
apiVersion: image.openshift.io/v1
metadata:
  name: support-log-gather-operator
spec:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bundle/manifests/stable/image-references` around lines 2 - 4, Add the
required metadata.name field to the ImageStream manifest alongside kind and
apiVersion, using the expected stable ImageStream name; leave the existing spec
unchanged.

@openshift-ci

openshift-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@neha037: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/validate-boilerplate 5406645 link false /test validate-boilerplate

Full PR test history. Your PR dashboard.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants