CONSOLE-5271: Set olmLifecycleEnabled based on OLMLifecycleAndCompatibility FeatureGate#1174
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@perdasilva: This pull request references CONSOLE-5271 which is a valid jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details🧰 Additional context used🔀 Multi-repo context openshift/consoleLinked repositories findingsopenshift/console
WalkthroughAdds OLM lifecycle metadata enablement to console config generation and derives it from the cluster FeatureGate in operator sync. ChangesOLM Lifecycle Metadata Feature Gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/console/operator/sync_v400.go (1)
586-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hardcoded feature-gate name with a constant.
"OLMLifecycleAndCompatibility"should be a shared named constant (ideally underpkg/api) to prevent typo drift across sync, builders, and tests.As per coding guidelines, "Use named constants instead of magic numbers and magic strings, with comments explaining their purpose."
🤖 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 `@pkg/console/operator/sync_v400.go` at line 586, The hardcoded string "OLMLifecycleAndCompatibility" in the condition where gate.Name is compared violates the coding guidelines that require using named constants instead of magic strings. Create a named constant for "OLMLifecycleAndCompatibility" in the pkg/api package to centralize this value and prevent typo drift across sync, builders, and tests. Then replace the hardcoded string in the gate.Name comparison with a reference to this new constant to ensure consistency across the codebase.Source: Coding guidelines
🤖 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
`@manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml`:
- Line 23: The nonResourceURLs pattern `/api/*/lifecycles/*` in the Kubernetes
RBAC role is invalid because wildcards cannot be used in the middle of a
path—only at the terminus. Replace this pattern with explicit versioned patterns
such as `/api/v1/lifecycles/*` and `/api/v2/lifecycles/*` (or whichever API
versions your operator needs to access), or use a single pattern matching your
target API version. Update the rule in the manifest to use only valid
trailing-suffix glob patterns.
---
Nitpick comments:
In `@pkg/console/operator/sync_v400.go`:
- Line 586: The hardcoded string "OLMLifecycleAndCompatibility" in the condition
where gate.Name is compared violates the coding guidelines that require using
named constants instead of magic strings. Create a named constant for
"OLMLifecycleAndCompatibility" in the pkg/api package to centralize this value
and prevent typo drift across sync, builders, and tests. Then replace the
hardcoded string in the gate.Name comparison with a reference to this new
constant to ensure consistency across the codebase.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2d836929-132c-4959-93a4-3fa12b797cde
⛔ Files ignored due to path filters (2)
pkg/console/subresource/configmap/configmap_test.gois excluded by!**/*_test.gopkg/console/subresource/configmap/tech_preview_test.gois excluded by!**/*_test.go
📒 Files selected for processing (5)
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yamlpkg/console/operator/sync_v400.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack context
**/*.go: Imports must be grouped with comments in the order: standard lib, 3rd party, kube, openshift, and operator (internal)
Wrap errors with context usingfmt.Errorf("failed to X: %w", err)pattern to provide meaningful error messages
**/*.go: Do NOT use deprecated ioutil package functions. Use os.ReadFile() instead of ioutil.ReadFile(), os.WriteFile() instead of ioutil.WriteFile(), and io.ReadAll() instead of ioutil.ReadAll() (Go 1.16+)
Do NOT use net.Dial() directly. Use (&net.Dialer{}).DialContext() to support context cancellation
Use %w verb in fmt.Errorf() to wrap errors and maintain error chains, not %v which loses the error chain
Add descriptive context to errors when wrapping them, including relevant identifiers and state information
Use specific error type checks (e.g., apierrors.IsNotFound()) instead of string matching with strings.Contains() on error.Error()
Always propagate the parent context in function calls instead of using context.Background(), to respect parent cont...
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
⚙️ CodeRabbit configuration file
**/*.go: Go security (prodsec-skills):
- Never ignore error returns
- database/sql with placeholders; no fmt.Sprintf in queries
- Use stdlib crypto/* and golang.org/x/crypto (Go team maintained);
avoid third-party crypto libraries- Integer overflow: bounds-check user-supplied sizes
- context.Context for cancellation and timeouts
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
pkg/console/subresource/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Use
pkg/console/subresource/packages for resource builders, with separate packages for each resource type (authentication, configmap, deployment, oauthclient, route, secret, etc.)
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.go
**
⚙️ CodeRabbit configuration file
**: # OpenShift Console Operator - AI Context HubThis file serves as the central AI documentation hub for the OpenShift Console Operator project. AI assistants (Claude, Cursor, Copilot, CodeRabbit, etc.) use this and the linked documents to understand project context.
Go Version and Dependencies
Go Version and Dependencies
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
Quick Reference
This Repository
Document Purpose ARCHITECTURE.md System architecture, components, repository structure CONVENTIONS.md Go coding standards, patterns, import organization TESTING.md Testing patterns, commands, debugging README.md Project README with setup instructions Console Repository (openshift/console)
For frontend-related guidelines, see the openshift/console repository:
Document Purpose STYLEGUIDE.md Frontend code style guidelines INTERNATIONALIZATION.md i18n patterns and translation guidelines CONTRIBUTING.md Contribution guidelines for the console project Project Summary
The **console-operator...
Files:
pkg/console/subresource/consoleserver/types.gomanifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yamlpkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.
Controllers should use the library-go factory pattern.
Status conditions should use status.Handle* functions.
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/operator/sync_v400.go
{manifests,bindata/assets,quickstarts,examples}/**/*.yaml
📄 CodeRabbit inference engine (.claude/skills/manifest-review.md)
{manifests,bindata/assets,quickstarts,examples}/**/*.yaml: CVO manifests MUST include cluster profile annotations: include.release.openshift.io/hypershift, include.release.openshift.io/ibm-cloud-managed, include.release.openshift.io/self-managed-high-availability, include.release.openshift.io/single-node-developer, and capability.openshift.io/name: Console
Console resources should include capability.openshift.io/name: Console annotation
Kubernetes apiGroups in RBAC manifests must be correct (empty string for core API, specific groups for CRDs)
Resources in manifests must maintain namespace consistency: console workload resources in openshift-console namespace, operator resources in openshift-console-operator namespace
YAML manifests must use proper indentation (2 spaces), consistent field ordering, and include --- separators between multiple resources
When binding roles to service accounts, use console-operator SA for cross-namespace permissions and console SA for same-namespace console workload bindings
New manifests must not have missing cluster profile annotations to ensure resources are deployed to appropriate clusters
Files:
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml
**/*.{yaml,yml,json}
📄 CodeRabbit inference engine (Custom checks)
**/*.{yaml,yml,json}: Flag privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN capability, running as root without justification, and allowPrivilegeEscalation: true in container/Kubernetes manifests
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology (3+ control-plane nodes, dedicated workers). Flag: required pod anti-affinity with maxUnavailable: 0 (deadlocks on SNO/TNF/TNA), pod topology spread with DoNotSchedule and hostname key (breaks on SNO), replica counts derived from node count without topology awareness, nodeSelector/node affinity targeting control-plane nodes (fails on HyperShift), scheduling to all control-plane nodes equally without excluding arbiter nodes (TNA), assuming dedicated worker nodes exist (SNO/TNF), or PodDisruptionBudgets designed for 3+ nodes (TNF/TNA). Do not flag if change checks ControlPlaneTopology, node counts, or topology labels before applying constraints.
Files:
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml
manifests/**/*.yaml
⚙️ CodeRabbit configuration file
manifests/**/*.yaml: Review CVO manifests carefully.
These are deployed to clusters and changes have wide impact.
Files:
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml
**/*.{yaml,yml}
⚙️ CodeRabbit configuration file
**/*.{yaml,yml}: If this is a Kubernetes/OpenShift manifest or Helm template:
- securityContext: runAsNonRoot, readOnlyRootFilesystem,
allowPrivilegeEscalation: false- Drop ALL capabilities, add only what is required
- Resource limits (cpu, memory) on every container
- No hostPID, hostNetwork, hostIPC, privileged: true
- NetworkPolicy defined for the namespace
- OpenShift: SCC must be restricted or custom-scoped
- Liveness + readiness probes defined
- automountServiceAccountToken: false unless needed
- RBAC: least privilege; no cluster-admin for workloads
- Helm: no .Values interpolation in shell commands
Files:
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
**/*sync*.go: Sync handler implementations must use incremental sync pattern where each reconciliation loop returns early on error rather than collecting errors and continuing
Sync resources in dependency order: ConfigMaps/Secrets → Service Accounts → RBAC (Roles, RoleBindings) → Services → Deployments → Routes
Return early on errors to maintain incremental sync behavior instead of continuing execution or logging and proceeding
Use resourceapply.Apply*() functions to handle both resource creation and update operations
Ensure deleted resources are cleaned up from the cluster when removed from config, with proper handling for NotFound errors
Check feature gates before syncing gated resources to ensure conditional resource reconciliation
Status updates must reflect actual reconciliation state by adding conditions as operations complete using status handlers with HandleProgressingOrDegraded and HandleAvailable
Respect ManagementState configurations and implement cleanup logic for Removed state
Avoid mutating live objects; instead build desired state separately before applying
Do not sync all resources regardless of errors; stop reconciliation on dependency failures
Files:
pkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400.go
🔀 Multi-repo context openshift/console
Based on my exploration of the openshift/console repository, I've identified important cross-repository context for this PR:
Linked repositories findings
openshift/console
Critical Finding: The PR target is NOT this repository
The files referenced in the PR context (pkg/console/operator/sync_v400.go, pkg/console/subresource/configmap/configmap.go, pkg/console/subresource/consoleserver/types.go) do not exist in the openshift/console repository. This PR is for a separate operator/configuration repository (likely openshift-api or console-operator) that generates the console-config YAML.
However, this PR has direct impact on openshift/console frontend:
Frontend changes required:
The openshift/console frontend needs to be updated to consume the new olmLifecycleEnabled flag. Currently:
-
File:
frontend/packages/operator-lifecycle-manager/src/features.ts[::openshift/console::]- Line 5 currently reads:
setFeatureFlag(Flags.OPERATOR_LIFECYCLE_METADATA, !!window.SERVER_FLAGS.techPreview); - This needs to be updated to read from the new
olmLifecycleEnabledfield when available
- Line 5 currently reads:
-
File:
frontend/@types/console/window.d.ts[::openshift/console::]- The
window.SERVER_FLAGSinterface does NOT currently include anolmLifecycleEnabledfield (lines 3-71) - This field must be added for TypeScript type safety
- The
-
Usage in:
frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx[::openshift/console::]- Uses
useFlag(Flags.OPERATOR_LIFECYCLE_METADATA)to conditionally render operator lifecycle metadata columns - Currently depends on the
techPreviewflag but should depend on the new dedicatedolmLifecycleEnabledflag
- Uses
Integration Pattern:
The implementation follows the existing pattern for techPreview:
- Backend reads cluster configuration (FeatureGate resource)
- Backend populates a flag in console-config YAML
- Flag gets serialized into
window.SERVER_FLAGSJSON in index.html (line 65) - Frontend feature handler reads the flag and sets Redux state
- Components use
useFlag()hook to access the feature flag
Required frontend synchronization:
- Update
window.d.tsto addolmLifecycleEnabled: booleanfield to SERVER_FLAGS - Update
features.tsto readwindow.SERVER_FLAGS.olmLifecycleEnabledinstead oftechPreview - Ensure backward compatibility if the field is missing
🔇 Additional comments (5)
pkg/console/subresource/consoleserver/types.go (1)
78-78: LGTM!pkg/console/subresource/consoleserver/config_builder.go (1)
86-86: LGTM!Also applies to: 315-319, 390-390
pkg/console/subresource/configmap/configmap.go (1)
52-52: LGTM!Also applies to: 70-70, 111-111
pkg/console/operator/sync_v400.go (1)
131-136: LGTM!Also applies to: 148-148, 345-345, 420-420
manifests/03-rbac-role-cluster-openshift-console-user-operator-lifecycle-reader.yaml (1)
15-20: LGTM!Also applies to: 32-37
perdasilva
force-pushed
the
CONSOLE-5271-olm-lifecycle-featuregate
branch
from
ba16e1d to
b157312
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/console/subresource/configmap/configmap_test.go (1)
1292-1309: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
DefaultConfigMaperrors instead of discarding them.Line 1292 ignores the returned error (
cm, _, _ := ...), which can hide setup failures and make later assertions crash oncm.Data.Suggested fix
- cm, _, _ := DefaultConfigMap( + cm, _, err := DefaultConfigMap( tt.args.operatorConfig, tt.args.consoleConfig, tt.args.authConfig, tt.args.managedConfig, @@ false, // techPreviewEnabled - default to false for tests false, // olmLifecycleMetadataEnabled - default to false for tests ) + if err != nil { + t.Fatalf("DefaultConfigMap() error = %v", err) + } // marshall the exampleYaml to map[string]interface{} so we can use it in diff below var exampleConfig map[string]interface{} exampleBytes := []byte(tt.want.Data[configKey]) - err := yaml.Unmarshal(exampleBytes, &exampleConfig) + err = yaml.Unmarshal(exampleBytes, &exampleConfig)As per coding guidelines:
**/*_test.go: “Check and handle all errors in tests using t.Fatalf() or t.Errorf(); do not ignore errors with blank identifiers.”🤖 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 `@pkg/console/subresource/configmap/configmap_test.go` around lines 1292 - 1309, The DefaultConfigMap function call at line 1292 is discarding the error return value using a blank identifier, which can hide setup failures in the test. Instead of using `cm, _, _ :=`, capture all three return values and check the error immediately after the function call using t.Fatalf() to fail the test if DefaultConfigMap returns an error, ensuring test setup failures are caught and not silently hidden.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/console/subresource/consoleserver/config_builder.go (1)
315-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd godoc for the exported setter.
OLMLifecycleMetadataEnabledis exported and should carry a godoc comment describing its behavior and return value.As per coding guidelines, "Document all exported functions with godoc comments explaining the function's purpose, parameters, and return values." As per path instructions, "Refer to /go-quality-review for all Go code to check ... Missing godoc on exported functions."
🤖 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 `@pkg/console/subresource/consoleserver/config_builder.go` around lines 315 - 319, The exported function OLMLifecycleMetadataEnabled is missing a godoc comment. Add a godoc comment directly above the function that starts with the function name and describes what the function does, its parameters, and its return value. The comment should follow the pattern of documenting the purpose of the setter, indicating that it sets the olmLifecycleMetadataEnabled field and returns a pointer to the ConsoleServerCLIConfigBuilder for method chaining.Sources: Coding guidelines, Path instructions
pkg/console/operator/sync_v400.go (1)
635-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the gate-name literal with a named constant.
Using a named constant here avoids string drift across call sites and makes the feature-gate contract easier to maintain.
As per coding guidelines, "Use named constants instead of magic numbers and magic strings, with comments explaining their purpose." As per path instructions, "Refer to /go-quality-review for all Go code to check ... Magic values: unexplained numbers/strings."
♻️ Proposed change
+const olmLifecycleAndCompatibilityGateName = "OLMLifecycleAndCompatibility" + func (co *consoleOperator) SyncOLMLifecycleMetadata() (olmLifecycleMetadataEnabled bool, reason string, err error) { @@ - if gate.Name == "OLMLifecycleAndCompatibility" { + if gate.Name == olmLifecycleAndCompatibilityGateName { klog.V(4).Infoln("OLM lifecycle metadata features enabled based on OLMLifecycleAndCompatibility feature gate.") return true, "", nil }🤖 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 `@pkg/console/operator/sync_v400.go` at line 635, The string literal "OLMLifecycleAndCompatibility" is being used directly in the gate name comparison instead of as a named constant. Define a named constant at the package level or in an appropriate constants file for this gate name, then replace the hardcoded string in the condition that checks `gate.Name == "OLMLifecycleAndCompatibility"` with a reference to that constant. This approach prevents string drift across multiple call sites and makes the feature-gate contract easier to maintain and update.Sources: Coding guidelines, Path instructions
🤖 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 `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1633-1731: Add test cases for nil input slices to both
table-driven tests TestPluginsWithI18nNamespaceSorting and
TestGetPluginsProxyServicesSorting. For each test function, add a new test case
in the tests slice where input is nil instead of an empty slice
[]*consolev1.ConsolePlugin{}, with an appropriate expected output (likely nil or
empty slice) to verify the functions handle nil inputs correctly as per the
edge-case testing guidelines.
In `@pkg/console/subresource/configmap/tech_preview_test.go`:
- Around line 83-146: The test TestOLMLifecycleMetadataEnabled codifies a new
backend flag but the frontend lacks corresponding support. To fix this, ensure
the backend properly exposes the OLMLifecycleMetadataEnabled flag in the server
flags response sent to the frontend, add proper TypeScript typing for
olmLifecycleEnabled in the frontend's SERVER_FLAGS interface, and update the
frontend code that currently sources OPERATOR_LIFECYCLE_METADATA from the
techPreview flag to instead use the new olmLifecycleEnabled flag from the
backend response. This ensures the backend and frontend flag contracts are
synchronized and the UI properly honors the new backend configuration.
In `@pkg/console/subresource/consoleserver/config_builder.go`:
- Line 390: The backend is adding olmLifecycleMetadataEnabled to the
configuration at the point where conf.OLMLifecycleMetadataEnabled is assigned,
but the frontend is not prepared to consume this new field. Add the
olmLifecycleEnabled field to the SERVER_FLAGS interface in the frontend type
definitions (specifically in window.d.ts where techPreview is currently
defined), and update the features.ts file where it currently reads
window.SERVER_FLAGS.techPreview to instead read the new olmLifecycleEnabled
field from SERVER_FLAGS. This ensures the backend configuration change can be
properly consumed by the frontend UI.
---
Outside diff comments:
In `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1292-1309: The DefaultConfigMap function call at line 1292 is
discarding the error return value using a blank identifier, which can hide setup
failures in the test. Instead of using `cm, _, _ :=`, capture all three return
values and check the error immediately after the function call using t.Fatalf()
to fail the test if DefaultConfigMap returns an error, ensuring test setup
failures are caught and not silently hidden.
---
Nitpick comments:
In `@pkg/console/operator/sync_v400.go`:
- Line 635: The string literal "OLMLifecycleAndCompatibility" is being used
directly in the gate name comparison instead of as a named constant. Define a
named constant at the package level or in an appropriate constants file for this
gate name, then replace the hardcoded string in the condition that checks
`gate.Name == "OLMLifecycleAndCompatibility"` with a reference to that constant.
This approach prevents string drift across multiple call sites and makes the
feature-gate contract easier to maintain and update.
In `@pkg/console/subresource/consoleserver/config_builder.go`:
- Around line 315-319: The exported function OLMLifecycleMetadataEnabled is
missing a godoc comment. Add a godoc comment directly above the function that
starts with the function name and describes what the function does, its
parameters, and its return value. The comment should follow the pattern of
documenting the purpose of the setter, indicating that it sets the
olmLifecycleMetadataEnabled field and returns a pointer to the
ConsoleServerCLIConfigBuilder for method chaining.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4eb3d0c0-ee4f-4fd5-a3a5-92fd23b128b0
📒 Files selected for processing (6)
pkg/console/operator/sync_v400.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
✅ Files skipped from review due to trivial changes (1)
- pkg/console/subresource/consoleserver/types.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack context
**/*.go: Imports must be grouped with comments in the order: standard lib, 3rd party, kube, openshift, and operator (internal)
Wrap errors with context usingfmt.Errorf("failed to X: %w", err)pattern to provide meaningful error messages
**/*.go: Do NOT use deprecated ioutil package functions. Use os.ReadFile() instead of ioutil.ReadFile(), os.WriteFile() instead of ioutil.WriteFile(), and io.ReadAll() instead of ioutil.ReadAll() (Go 1.16+)
Do NOT use net.Dial() directly. Use (&net.Dialer{}).DialContext() to support context cancellation
Use %w verb in fmt.Errorf() to wrap errors and maintain error chains, not %v which loses the error chain
Add descriptive context to errors when wrapping them, including relevant identifiers and state information
Use specific error type checks (e.g., apierrors.IsNotFound()) instead of string matching with strings.Contains() on error.Error()
Always propagate the parent context in function calls instead of using context.Background(), to respect parent cont...
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.Refer to the following skills based on CODE PATTERNS, not just file paths:
Refer to /controller-review when code contains:
- Controller struct types (e.g.,
type *Controller struct)func New*Controller(factory functionsfactory.New().WithFilteredEventsInformers(pattern.ToController(method callsSync(ctx context.Context, controllerContext factory.SyncContext)methodsoperatorConfig.Spec.ManagementStatechecksstatus.NewStatusHandlerorstatus.Handle*functionsRefer to /sync-handler-review when code contains:
- Main operator sync functions (e.g.,
sync_v400.gocontent)- Sequential resource syncing with early returns
- Incremental reconciliation loops
- Multiple
resourceapply.Apply*()calls in sequence- Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes
- Feature gate conditional logic
Refer to /go-quality-review for all Go code to check:
- Deprecated imports:
ioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll- Deprecated patterns:
DialwithoutDialContext- Error handling: missing
%win fmt.Errorf- Code smells: deep nesting (4+ levels), functions >100 lines
- Magic values: unexplained numbers/strings
- Context propagation:
context.Background()instead of passed ctx- Missing godoc on exported functions
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
pkg/console/subresource/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Use
pkg/console/subresource/packages for resource builders, with separate packages for each resource type (authentication, configmap, deployment, oauthclient, route, secret, etc.)
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
**
⚙️ CodeRabbit configuration file
**: # OpenShift Console Operator - AI Context HubThis file serves as the central AI documentation hub for the OpenShift Console Operator project. AI assistants (Claude, Cursor, Copilot, CodeRabbit, etc.) use this and the linked documents to understand project context.
Go Version and Dependencies
Go Version and Dependencies
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
Quick Reference
This Repository
Document Purpose ARCHITECTURE.md System architecture, components, repository structure CONVENTIONS.md Go coding standards, patterns, import organization TESTING.md Testing patterns, commands, debugging README.md Project README with setup instructions Console Repository (openshift/console)
For frontend-related guidelines, see the openshift/console repository:
Document Purpose STYLEGUIDE.md Frontend code style guidelines INTERNATIONALIZATION.md i18n patterns and translation guidelines CONTRIBUTING.md Contribution guidelines for the console project Project Summary
The **console-operator...
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/tech_preview_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow testing patterns and commands documented in TESTING.md
Follow testing patterns and commands as documented in TESTING.md, including running unit tests with 'make test-unit' and checks with 'make check'
**/*_test.go: Use table-driven tests for comprehensive coverage
Usehttptestfor HTTP handler testing in Go
Include proper cleanup functions in tests
Test both success and failure pathsCheck and handle all errors in tests using t.Fatalf() or t.Errorf(); do not ignore errors with blank identifiers
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Review test code for quality and patterns.Refer to /unit-test-review when test is in pkg//*_test.go:**
- Table-driven test structure with test cases
- Use of
go-test/deepfor struct comparisons- Test naming conventions (TestFunctionName)
- Error handling with
wantErrpattern- Edge case coverage (nil, empty, boundary values)
- Proper assertions with helpful error messages
- Test isolation (no shared mutable state)
Refer to /e2e-test-review when test contains:
framework.MustNewClientset(t, nil)or similar e2e framework usagewait.Pollorwait.PollImmediatepatternsretry.RetryOnConflictfor updates- Cleanup via
deferfunctions- Console/operator CR manipulations
- Test assertions on cluster state
Suggest to use /e2e-test-review when:
- PR adds new feature requiring e2e coverage
- Test file is empty or skeleton
- Comments indicate "TODO: add test"
Review for common issues:
- Missing cleanup (defer statements)
- Using
time.Sleepinstead ofwait.Poll- Missing context timeouts
- Vague error messages in assertions
- Tests without table-driven structure when testing multiple cases
- Ignoring errors with
_- Tests without assertions
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (.claude/skills/unit-test-review.md)
pkg/**/*_test.go: Go unit tests in pkg/**/*_test.go files should use table-driven test pattern for clarity and completeness with struct containing test cases
Test function names should be descriptive and clearly indicate what is being tested (e.g., TestGetNodeComputeEnvironments, TestNewRouteConfig)
Test case names within table-driven tests should explain the scenario being tested (e.g., 'Custom hostname and TLS secret set'), not be vague (e.g., 'test1')
Use github.com/go-test/deep package for struct comparisons to show exact differences instead of simple equality checks or manual field-by-field comparisons
Test both success and failure paths, including edge cases such as empty inputs (nil, "", empty slices/maps), boundary values (0, -1, max int), missing labels/fields, duplicate values, and large inputs
Organize test code using Arrange-Act-Assert pattern: setup test data in Arrange phase, call the function in Act phase, verify results in Assert phase
Check error presence using pattern (err != nil) != tt.wantErr instead of ignoring errors with underscore or missing error checks
When checking specific error messages, use strings.Contains to verify error content instead of just checking error presence
Extract common test setup into helper functions rather than duplicating setup code across multiple tests
Write specific assertion error messages with context (e.g., 'expected %d nodes, got %d') rather than vague messages (e.g., 'failed')
Inline simple test data in test structs; extract complex fixtures into testdata files or helper functions
Avoid red flags in tests: no table-driven tests for multi-scenario functions, tests without subtests, tests depending on execution order, global mutable state, hardcoded sleeps, tests without assertions, ignoring errors with underscore, testing implementation details instead of behavior, or overly complex test setup
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
**/*sync*.go: Sync handler implementations must use incremental sync pattern where each reconciliation loop returns early on error rather than collecting errors and continuing
Sync resources in dependency order: ConfigMaps/Secrets → Service Accounts → RBAC (Roles, RoleBindings) → Services → Deployments → Routes
Return early on errors to maintain incremental sync behavior instead of continuing execution or logging and proceeding
Use resourceapply.Apply*() functions to handle both resource creation and update operations
Ensure deleted resources are cleaned up from the cluster when removed from config, with proper handling for NotFound errors
Check feature gates before syncing gated resources to ensure conditional resource reconciliation
Status updates must reflect actual reconciliation state by adding conditions as operations complete using status handlers with HandleProgressingOrDegraded and HandleAvailable
Respect ManagementState configurations and implement cleanup logic for Removed state
Avoid mutating live objects; instead build desired state separately before applying
Do not sync all resources regardless of errors; stop reconciliation on dependency failures
Files:
pkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400.go
🔀 Multi-repo context openshift/console
Based on my comprehensive research of the openshift/console repository, I have confirmed a critical integration gap that reviewers need to be aware of.
Linked repositories findings
openshift/console [::openshift/console::]
Critical Finding: Frontend Not Yet Updated for OLMLifecycleMetadataEnabled Integration
The PR being reviewed adds backend support for passing olmLifecycleMetadataEnabled through the console-config.yaml pipeline. However, the openshift/console frontend repository does not yet have the complementary changes needed to consume this new field.
Missing Type Definition:
- File:
frontend/@types/console/window.d.ts[::openshift/console::] - Current state: The
SERVER_FLAGSinterface (line 66) includestechPreview: booleanbut does NOT include anolmLifecycleEnabledfield - Required change: Add
olmLifecycleEnabled: boolean;to the SERVER_FLAGS interface to match the backend's new config field
Incorrect Feature Flag Handler:
- File:
frontend/packages/operator-lifecycle-manager/src/features.ts[::openshift/console::] - Line 5 (current):
setFeatureFlag(Flags.OPERATOR_LIFECYCLE_METADATA, !!window.SERVER_FLAGS.techPreview); - Problem: Still reads from
techPreviewinstead of the newolmLifecycleEnabledfield - Required change: Update to read
!!window.SERVER_FLAGS.olmLifecycleEnabled(with fallback for backward compatibility if the field is missing)
Component Usage:
- File:
frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx[::openshift/console::] - Uses
useFlag(Flags.OPERATOR_LIFECYCLE_METADATA)to conditionally render operator lifecycle metadata columns - Will work correctly once the feature flag handler is updated
Integration Risk:
Without these frontend updates, the backend PR will generate the olmLifecycleMetadataEnabled field in console-config.yaml, but the frontend won't be able to read it due to missing type definitions and the handler still reading from the old techPreview field. This creates a version mismatch between backend and frontend configurations.
🔇 Additional comments (5)
pkg/console/subresource/consoleserver/config_builder.go (1)
86-86: LGTM!pkg/console/subresource/configmap/configmap.go (1)
52-52: LGTM!Also applies to: 70-70, 111-111, 183-185, 198-201, 223-227
pkg/console/operator/sync_v400.go (1)
9-9: LGTM!Also applies to: 36-36, 133-138, 150-150, 221-223, 266-291, 329-346, 362-362, 437-457, 524-562, 564-606, 624-634, 636-642, 810-825, 827-838, 896-903
pkg/console/subresource/configmap/configmap_test.go (1)
981-984: LGTM!pkg/console/subresource/configmap/tech_preview_test.go (1)
45-81: LGTM!Also applies to: 148-181
There was a problem hiding this comment.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/console/subresource/configmap/configmap_test.go (1)
1292-1309: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
DefaultConfigMaperrors instead of discarding them.Line 1292 ignores the returned error (
cm, _, _ := ...), which can hide setup failures and make later assertions crash oncm.Data.Suggested fix
- cm, _, _ := DefaultConfigMap( + cm, _, err := DefaultConfigMap( tt.args.operatorConfig, tt.args.consoleConfig, tt.args.authConfig, tt.args.managedConfig, @@ false, // techPreviewEnabled - default to false for tests false, // olmLifecycleMetadataEnabled - default to false for tests ) + if err != nil { + t.Fatalf("DefaultConfigMap() error = %v", err) + } // marshall the exampleYaml to map[string]interface{} so we can use it in diff below var exampleConfig map[string]interface{} exampleBytes := []byte(tt.want.Data[configKey]) - err := yaml.Unmarshal(exampleBytes, &exampleConfig) + err = yaml.Unmarshal(exampleBytes, &exampleConfig)As per coding guidelines:
**/*_test.go: “Check and handle all errors in tests using t.Fatalf() or t.Errorf(); do not ignore errors with blank identifiers.”🤖 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 `@pkg/console/subresource/configmap/configmap_test.go` around lines 1292 - 1309, The DefaultConfigMap function call at line 1292 is discarding the error return value using a blank identifier, which can hide setup failures in the test. Instead of using `cm, _, _ :=`, capture all three return values and check the error immediately after the function call using t.Fatalf() to fail the test if DefaultConfigMap returns an error, ensuring test setup failures are caught and not silently hidden.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/console/subresource/consoleserver/config_builder.go (1)
315-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd godoc for the exported setter.
OLMLifecycleMetadataEnabledis exported and should carry a godoc comment describing its behavior and return value.As per coding guidelines, "Document all exported functions with godoc comments explaining the function's purpose, parameters, and return values." As per path instructions, "Refer to /go-quality-review for all Go code to check ... Missing godoc on exported functions."
🤖 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 `@pkg/console/subresource/consoleserver/config_builder.go` around lines 315 - 319, The exported function OLMLifecycleMetadataEnabled is missing a godoc comment. Add a godoc comment directly above the function that starts with the function name and describes what the function does, its parameters, and its return value. The comment should follow the pattern of documenting the purpose of the setter, indicating that it sets the olmLifecycleMetadataEnabled field and returns a pointer to the ConsoleServerCLIConfigBuilder for method chaining.Sources: Coding guidelines, Path instructions
pkg/console/operator/sync_v400.go (1)
635-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the gate-name literal with a named constant.
Using a named constant here avoids string drift across call sites and makes the feature-gate contract easier to maintain.
As per coding guidelines, "Use named constants instead of magic numbers and magic strings, with comments explaining their purpose." As per path instructions, "Refer to /go-quality-review for all Go code to check ... Magic values: unexplained numbers/strings."
♻️ Proposed change
+const olmLifecycleAndCompatibilityGateName = "OLMLifecycleAndCompatibility" + func (co *consoleOperator) SyncOLMLifecycleMetadata() (olmLifecycleMetadataEnabled bool, reason string, err error) { @@ - if gate.Name == "OLMLifecycleAndCompatibility" { + if gate.Name == olmLifecycleAndCompatibilityGateName { klog.V(4).Infoln("OLM lifecycle metadata features enabled based on OLMLifecycleAndCompatibility feature gate.") return true, "", nil }🤖 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 `@pkg/console/operator/sync_v400.go` at line 635, The string literal "OLMLifecycleAndCompatibility" is being used directly in the gate name comparison instead of as a named constant. Define a named constant at the package level or in an appropriate constants file for this gate name, then replace the hardcoded string in the condition that checks `gate.Name == "OLMLifecycleAndCompatibility"` with a reference to that constant. This approach prevents string drift across multiple call sites and makes the feature-gate contract easier to maintain and update.Sources: Coding guidelines, Path instructions
🤖 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 `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1633-1731: Add test cases for nil input slices to both
table-driven tests TestPluginsWithI18nNamespaceSorting and
TestGetPluginsProxyServicesSorting. For each test function, add a new test case
in the tests slice where input is nil instead of an empty slice
[]*consolev1.ConsolePlugin{}, with an appropriate expected output (likely nil or
empty slice) to verify the functions handle nil inputs correctly as per the
edge-case testing guidelines.
In `@pkg/console/subresource/configmap/tech_preview_test.go`:
- Around line 83-146: The test TestOLMLifecycleMetadataEnabled codifies a new
backend flag but the frontend lacks corresponding support. To fix this, ensure
the backend properly exposes the OLMLifecycleMetadataEnabled flag in the server
flags response sent to the frontend, add proper TypeScript typing for
olmLifecycleEnabled in the frontend's SERVER_FLAGS interface, and update the
frontend code that currently sources OPERATOR_LIFECYCLE_METADATA from the
techPreview flag to instead use the new olmLifecycleEnabled flag from the
backend response. This ensures the backend and frontend flag contracts are
synchronized and the UI properly honors the new backend configuration.
In `@pkg/console/subresource/consoleserver/config_builder.go`:
- Line 390: The backend is adding olmLifecycleMetadataEnabled to the
configuration at the point where conf.OLMLifecycleMetadataEnabled is assigned,
but the frontend is not prepared to consume this new field. Add the
olmLifecycleEnabled field to the SERVER_FLAGS interface in the frontend type
definitions (specifically in window.d.ts where techPreview is currently
defined), and update the features.ts file where it currently reads
window.SERVER_FLAGS.techPreview to instead read the new olmLifecycleEnabled
field from SERVER_FLAGS. This ensures the backend configuration change can be
properly consumed by the frontend UI.
---
Outside diff comments:
In `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1292-1309: The DefaultConfigMap function call at line 1292 is
discarding the error return value using a blank identifier, which can hide setup
failures in the test. Instead of using `cm, _, _ :=`, capture all three return
values and check the error immediately after the function call using t.Fatalf()
to fail the test if DefaultConfigMap returns an error, ensuring test setup
failures are caught and not silently hidden.
---
Nitpick comments:
In `@pkg/console/operator/sync_v400.go`:
- Line 635: The string literal "OLMLifecycleAndCompatibility" is being used
directly in the gate name comparison instead of as a named constant. Define a
named constant at the package level or in an appropriate constants file for this
gate name, then replace the hardcoded string in the condition that checks
`gate.Name == "OLMLifecycleAndCompatibility"` with a reference to that constant.
This approach prevents string drift across multiple call sites and makes the
feature-gate contract easier to maintain and update.
In `@pkg/console/subresource/consoleserver/config_builder.go`:
- Around line 315-319: The exported function OLMLifecycleMetadataEnabled is
missing a godoc comment. Add a godoc comment directly above the function that
starts with the function name and describes what the function does, its
parameters, and its return value. The comment should follow the pattern of
documenting the purpose of the setter, indicating that it sets the
olmLifecycleMetadataEnabled field and returns a pointer to the
ConsoleServerCLIConfigBuilder for method chaining.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4eb3d0c0-ee4f-4fd5-a3a5-92fd23b128b0
📒 Files selected for processing (6)
pkg/console/operator/sync_v400.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
✅ Files skipped from review due to trivial changes (1)
- pkg/console/subresource/consoleserver/types.go
📜 Review details
🔇 Additional comments (5)
pkg/console/subresource/consoleserver/config_builder.go (1)
86-86: LGTM!pkg/console/subresource/configmap/configmap.go (1)
52-52: LGTM!Also applies to: 70-70, 111-111, 183-185, 198-201, 223-227
pkg/console/operator/sync_v400.go (1)
9-9: LGTM!Also applies to: 36-36, 133-138, 150-150, 221-223, 266-291, 329-346, 362-362, 437-457, 524-562, 564-606, 624-634, 636-642, 810-825, 827-838, 896-903
pkg/console/subresource/configmap/configmap_test.go (1)
981-984: LGTM!pkg/console/subresource/configmap/tech_preview_test.go (1)
45-81: LGTM!Also applies to: 148-181
🛑 Comments failed to post (3)
pkg/console/subresource/configmap/configmap_test.go (1)
1633-1731: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add
nilinput cases to the new sorting tests.These table-driven tests cover non-empty and empty slices, but they miss
nilplugin slice inputs, which are explicitly called out in test edge-case guidance and are common in Go call paths.As per coding guidelines:
pkg/**/*_test.go: “Test both success and failure paths, including edge cases such as empty inputs (nil, "", empty slices/maps)…”🤖 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 `@pkg/console/subresource/configmap/configmap_test.go` around lines 1633 - 1731, Add test cases for nil input slices to both table-driven tests TestPluginsWithI18nNamespaceSorting and TestGetPluginsProxyServicesSorting. For each test function, add a new test case in the tests slice where input is nil instead of an empty slice []*consolev1.ConsolePlugin{}, with an appropriate expected output (likely nil or empty slice) to verify the functions handle nil inputs correctly as per the edge-case testing guidelines.Source: Coding guidelines
pkg/console/subresource/configmap/tech_preview_test.go (1)
83-146: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backend/frontend flag contract appears out of sync for OLM lifecycle metadata.
This test codifies
ClusterInfo.OLMLifecycleMetadataEnabled, but linkedopenshift/consolefindings show frontend still lacksSERVER_FLAGS.olmLifecycleEnabledtyping and still sourcesOPERATOR_LIFECYCLE_METADATAfromtechPreview. Without coordinated compatibility, the UI won’t honor this new backend flag.🤖 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 `@pkg/console/subresource/configmap/tech_preview_test.go` around lines 83 - 146, The test TestOLMLifecycleMetadataEnabled codifies a new backend flag but the frontend lacks corresponding support. To fix this, ensure the backend properly exposes the OLMLifecycleMetadataEnabled flag in the server flags response sent to the frontend, add proper TypeScript typing for olmLifecycleEnabled in the frontend's SERVER_FLAGS interface, and update the frontend code that currently sources OPERATOR_LIFECYCLE_METADATA from the techPreview flag to instead use the new olmLifecycleEnabled flag from the backend response. This ensures the backend and frontend flag contracts are synchronized and the UI properly honors the new backend configuration.Source: Linked repositories
pkg/console/subresource/consoleserver/config_builder.go (1)
390-390: 🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # First, examine the config_builder.go file around line 390 to understand the context echo "=== Examining config_builder.go around line 390 ===" sed -n '380,400p' pkg/console/subresource/consoleserver/config_builder.go echo "" echo "=== Full context of the Build() function ===" ast-grep outline pkg/console/subresource/consoleserver/config_builder.goRepository: openshift/console-operator
Length of output: 7109
🏁 Script executed:
#!/bin/bash # Search for OLMLifecycleMetadataEnabled in the backend codebase echo "=== Searching for OLMLifecycleMetadataEnabled references ===" rg "OLMLifecycleMetadataEnabled" --type go -B 2 -A 2Repository: openshift/console-operator
Length of output: 225
🏁 Script executed:
#!/bin/bash # Verify the linked findings about the frontend - check SERVER_FLAGS type definition echo "=== Checking SERVER_FLAGS interface in window.d.ts ===" grep -n "SERVER_FLAGS\|olmLifecycleEnabled\|techPreview" frontend/@types/console/window.d.ts | head -20 echo "" echo "=== Checking features.ts line 5 ===" sed -n '1,15p' frontend/packages/operator-lifecycle-manager/src/features.tsRepository: openshift/console
Length of output: 602
🏁 Script executed:
#!/bin/bash # Check if there are any recent changes or comments about olmLifecycleEnabled integration echo "=== Searching for olmLifecycleEnabled in frontend ===" rg "olmLifecycle" --type ts --type tsx -B 2 -A 2 || echo "No matches found" echo "" echo "=== Searching for OPERATOR_LIFECYCLE_METADATA flag usage ===" rg "OPERATOR_LIFECYCLE_METADATA" --type ts --type tsx -B 2 -A 2Repository: openshift/console
Length of output: 344
Coordinate rollout with the frontend contract before release.
Line 390 propagates
OLMLifecycleMetadataEnabledinto generated config. However, the frontend repository lacks complementary changes needed to consume this field:
SERVER_FLAGSinterface infrontend/@types/console/window.d.ts(line 66) definestechPreview: booleanbut is missing theolmLifecycleEnabledfieldfrontend/packages/operator-lifecycle-manager/src/features.ts(line 5) still readswindow.SERVER_FLAGS.techPreviewinstead of the new fieldWithout these frontend updates, the backend flag will be generated but remain unread, preventing the UI behavior from activating. Requires lockstep frontend and backend changes.
🤖 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 `@pkg/console/subresource/consoleserver/config_builder.go` at line 390, The backend is adding olmLifecycleMetadataEnabled to the configuration at the point where conf.OLMLifecycleMetadataEnabled is assigned, but the frontend is not prepared to consume this new field. Add the olmLifecycleEnabled field to the SERVER_FLAGS interface in the frontend type definitions (specifically in window.d.ts where techPreview is currently defined), and update the features.ts file where it currently reads window.SERVER_FLAGS.techPreview to instead read the new olmLifecycleEnabled field from SERVER_FLAGS. This ensures the backend configuration change can be properly consumed by the frontend UI.Source: Linked repositories
perdasilva
force-pushed
the
CONSOLE-5271-olm-lifecycle-featuregate
branch
2 times, most recently
from
0d45934 to
45835d3
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
…dCompatibility FeatureGate Read the cluster FeatureGate resource and check whether the OLMLifecycleAndCompatibility gate is enabled in status.featureGates. Pass the result as olmLifecycleMetadataEnabled in the ConsoleConfig ClusterInfo so the console frontend can show/hide operator lifecycle metadata columns without requiring user RBAC to read FeatureGate resources. - Add SyncOLMLifecycleMetadata() to check status.featureGates[].enabled for the OLMLifecycleAndCompatibility gate name - Add OLMLifecycleMetadataEnabled field to ClusterInfo and config builder - Thread olmLifecycleMetadataEnabled through SyncConfigMap → DefaultConfigMap - Add unit tests for OLMLifecycleMetadataEnabled config generation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
CONSOLE-5271-olm-lifecycle-featuregate
branch
from
45835d3 to
cb44e79
Compare
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva, TheRealJon 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 |
openshift-ci
Bot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
Docs Approver: PX Approver: |
|
/retest |
|
@perdasilva: 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. |
3 participants
Analysis / Root cause:
The operator lifecycle metadata UI in the console (cluster compatibility and support phase columns on the Installed Operators page) needs to be gated on the
OLMLifecycleAndCompatibilitycluster FeatureGate. The console frontend cannot safely read theFeatureGateresource directly because not all users have RBAC forconfig.openshift.io/v1 FeatureGate. The console-operator, which runs with a service account, is the correct place to read the FeatureGate and pass the result to the console via the ConsoleConfig.Solution description:
SyncOLMLifecycle()method that reads the clusterFeatureGateresource and checksstatus.featureGates[].enabled[]for theOLMLifecycleAndCompatibilitygate nameOLMLifecycleEnabledfield toClusterInfostruct and theConsoleServerCLIConfigBuilderolmLifecycleEnabledthroughSyncConfigMap()→DefaultConfigMap()→ both config builder chainsconsole-config.yamlnow includesclusterInfo.olmLifecycleEnabled: truewhen the gate is enabledTest setup:
Requires a cluster with the
OLMLifecycleAndCompatibilityFeatureGate enabled.Test cases:
olmLifecycleEnabled: trueappears in the console ConfigMap when theOLMLifecycleAndCompatibilitygate is enabled in the cluster FeatureGateolmLifecycleEnabledis absent/false when the gate is not enabledDefaultConfigMapwitholmLifecycleEnabledtrue and falseBrowser conformance:
Additional info:
Companion PR to openshift/console#16668 which adds the
--olm-lifecyclebridge flag and readswindow.SERVER_FLAGS.olmLifecyclein the frontend.Reviewers and assignees:
/assign @perdasilva
Summary by CodeRabbit
OLMLifecycleAndCompatibilityfeature gate and propagates the resulting enablement into the generated console configuration, including updated console-config YAML.OLMLifecycleMetadataEnabledtoggle is correctly set and reflected after unmarshaling.