Draft
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
WalkthroughThis PR introduces a new Proposal feature for the cluster-version-operator, adding comprehensive Kubernetes CRD definitions for proposal-based cluster update workflows. A new proposal controller manages the lifecycle of proposals, generating them from available updates and managing their retention. Feature gates are extended, the operator is wired to instantiate and run the proposal controller when enabled, and comprehensive tests and API types are added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
pkg/cvo/availableupdates_test.go (1)
247-255: Extract this proposal-controller stub into a shared test helper.The same constructor/callback bundle is copied into three tests. A small helper keeps the stub behavior aligned and avoids missing one call site when
proposal.NewController(...)changes again.Also applies to: 345-353, 790-798
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/availableupdates_test.go` around lines 247 - 255, Extract the repeated proposal.NewController(...) stub into a single test helper function (e.g., newTestProposalController or makeStubProposalController) and replace each inline instantiation assigned to optr.proposalController with a call to that helper; the helper should return the same controller configured with the three simple callbacks used (returning nils, empty ClusterVersion, empty ConfigMap, and optr.release.Version) so all occurrences (including the other two places where proposal.NewController is duplicated) remain consistent when the constructor signature changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cvo/cvo.go`:
- Around line 327-335: The ConfigMap getter passed into proposal.NewController
has its parameters reversed: the closure is declared as func(name, namespace
string) but proposal.Controller calls it as (namespace, name), causing inverted
lookups; fix by changing the closure signature to func(namespace, name string)
(*corev1.ConfigMap, error) and ensure it calls
cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name) with the
corrected parameter order so optr.proposalController can fetch prompt ConfigMaps
correctly.
In `@pkg/cvo/status.go`:
- Around line 185-191: The code reads original.Status.History without checking
original (or original.Status), which can panic; update the conditional in the
block guarded by optr.shouldEnableProposalController() to first ensure original
is non-nil and original.Status is non-nil (or otherwise treat original history
length as 0) before comparing lengths with config.Status.History, then only call
optr.proposalController.Queue().Add(...) when the safe length comparison
indicates pruning; reference optr.shouldEnableProposalController,
config.Status.History, original.Status.History, and
optr.proposalController.Queue().Add to locate and fix the check.
In `@pkg/proposal/api/v1alpha1/agent_types.go`:
- Around line 197-255: AgentSpec documents that OutputFields and RawOutputSchema
are mutually exclusive but the CRD doesn't enforce it; implement validation in
the API type by adding webhook validation on the Agent (implement
webhook.Validator methods ValidateCreate and ValidateUpdate) that checks
AgentSpec.OutputFields and AgentSpec.RawOutputSchema and returns a validation
error if both are non-empty; reference the AgentSpec struct, the OutputFields
slice and RawOutputSchema *apiextensionsv1.JSON, and add identical checks in
both ValidateCreate and ValidateUpdate to reject requests that set both fields.
- Around line 132-145: OutputFieldItems currently allows Type=object without
Properties and allows Properties for primitive Types; add CRD validation rules
on OutputFieldItems to enforce that when Type (OutputFieldType) == "object" the
Properties ([]OutputSubField) is required and non-empty, and when Type !=
"object" Properties must be omitted/empty; implement this by adding kubebuilder
x-validation markers (XValidation) on the OutputFieldItems struct that check
Type == "object" implies Properties is present and has length > 0, and Type !=
"object" implies Properties is null/empty, so the controller will only receive
sensible object-item schemas (also update the struct comment for
OutputFieldItems to reflect the stricter validation).
In `@pkg/proposal/api/v1alpha1/proposal_types.go`:
- Around line 658-677: The Proposal CRD documentation and behavior are
inconsistent: the controller creates Proposals with a namespace but the comment
says "Proposal is cluster-scoped". Update the Proposal type to declare the
correct namespaced scope by adding the kubebuilder marker
+kubebuilder:resource:scope=Namespaced to the Proposal struct and change the doc
comment text from "Proposal is cluster-scoped" to reflect that Proposals are
namespaced; also verify pkg/proposal/controller.go still creates Proposals with
a namespace and adjust any creation logic only if you intend to make Proposals
cluster-scoped instead.
In `@pkg/proposal/api/v1alpha1/workflow_types.go`:
- Around line 166-168: Add reciprocal XValidation rules to enforce the
documented "skip => no agentRef" invariant on WorkflowStep so that when
analysis.skip, execution.skip, or verification.skip is true the corresponding
agentRef must be absent/empty; update the kubebuilder annotations near the
existing rules that reference
self.analysis.skip/self.execution.skip/self.verification.skip to also include
XValidation rules that assert not has(self.<section>.agentRef) or that
self.<section>.agentRef.name is empty (using the same self.analysis.agentRef,
self.execution.agentRef, self.verification.agentRef symbols to locate the
fields).
In `@pkg/proposal/controller.go`:
- Around line 157-160: The getProposals error is only logged in Sync, so
failures are not propagated and prevent retries; update the Sync call site to
return the error instead of just logging it: when getProposals (function
getProposals) returns a non-nil err, wrap or annotate the error with context
(e.g., "getting proposals for channel X") and return it from Sync so the
reconciler sees the failure and can retry; remove or keep logging as a
debug/info entry but do not suppress propagation — ensure the caller of Sync
continues to receive the error.
- Around line 124-126: The code returns early when both updates and
conditionalUpdates are empty, skipping cleanup; ensure stale CVO-owned proposals
are pruned by calling deleteProposals(...) before any early return or by
restructuring the logic so deleteProposals is always invoked when updates and
conditionalUpdates are empty (reference the variables updates,
conditionalUpdates and the deleteProposals function in the controller.go flow).
- Around line 135-138: The call to c.configMapGetterFunc in controller.go passes
the arguments in the wrong order (currently c.config.Namespace,
c.config.PromptConfigMap) even though the function signature is (name,
namespace); change the call to pass the prompt ConfigMap name first and the
namespace second (use c.config.PromptConfigMap, c.config.Namespace) so
promptConfigMap, err := c.configMapGetterFunc(...) queries the correct object
and reconciliation succeeds.
- Around line 187-193: The call to c.client.Create(ctx, proposal) discards its
return error and the code incorrectly checks the outer err; change it to capture
the error (e.g. err := c.client.Create(ctx, proposal)) and then use that local
err for the kerrors.IsAlreadyExists check, the klog.V(...).Infof messages, and
when appending to errs so real API failures are logged and recorded; update all
references in this if/else branch that currently use the stale err to use the
newly captured err instead.
In `@test/cvo/proposal.go`:
- Around line 95-97: The test case invoked via g.It currently reads "should
create proposals" and is gated by util.SkipIfNotTechPreviewNoUpgrade but lacks
the TechPreview label and the Jira-prefixed name; update the g.It call so the
test name follows the Jira format (e.g., `[Jira:"Cluster Version Operator"]
should create proposals`) and add the TechPreview label in the label list (e.g.,
g.Label("Serial"), g.Label("TechPreview")) while keeping oteginkgo.Informing()
and the existing util.SkipIfNetworkRestricted/util.SkipIfNotTechPreviewNoUpgrade
checks intact.
- Around line 110-119: The current poll returns true as soon as any Proposal
exists in external.DefaultCVONamespace; change this to wait for the specific new
Proposal(s) by first capturing the existing proposals via
rtClient.List(&proposalv1alpha1.ProposalList{}) to record the initial set/count,
then use wait.PollUntilContextTimeout to either (a) check that the count has
increased beyond the recorded initial count, or (b) list and filter for the
expected proposal identity (by name or label) and return true only when that
specific Proposal is present; update the polling closure that currently calls
rtClient.List(...) against external.DefaultCVONamespace accordingly to compare
against the initial snapshot or to match the expected name/label.
---
Nitpick comments:
In `@pkg/cvo/availableupdates_test.go`:
- Around line 247-255: Extract the repeated proposal.NewController(...) stub
into a single test helper function (e.g., newTestProposalController or
makeStubProposalController) and replace each inline instantiation assigned to
optr.proposalController with a call to that helper; the helper should return the
same controller configured with the three simple callbacks used (returning nils,
empty ClusterVersion, empty ConfigMap, and optr.release.Version) so all
occurrences (including the other two places where proposal.NewController is
duplicated) remain consistent when the constructor signature changes.
🪄 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: c1db13a3-da5d-497d-a393-1882fe85534c
⛔ Files ignored due to path filters (53)
go.sumis excluded by!**/*.sum,!go.sumpkg/proposal/api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/evanphx/json-patch/v5/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fold.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fuzz.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/indent.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/scanner.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tables.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tags.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/merge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/patch.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/metadata/interface.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/metadata/metadata.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/apimachinery.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/errors.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/restmapper.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/applyconfigurations.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/codec.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/config/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/dryrun.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/typeconverter.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fieldowner.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fieldvalidation.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor/intercept.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/metadata_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/namespaced_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/object.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/options.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/patch.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/typed_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/unstructured_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/watch.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/field/selector/utils.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/objectutil/objectutil.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/deleg.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/null.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/warning_handler.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (23)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsongo.modpkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featurechangestopper_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/payload/precondition/clusterversion/upgradeable.gopkg/proposal/api/v1alpha1/agent_types.gopkg/proposal/api/v1alpha1/groupversion_info.gopkg/proposal/api/v1alpha1/llmprovider_types.gopkg/proposal/api/v1alpha1/olsconfig_types.gopkg/proposal/api/v1alpha1/outputfield_deepcopy.gopkg/proposal/api/v1alpha1/proposal_types.gopkg/proposal/api/v1alpha1/workflow_types.gopkg/proposal/controller.gopkg/proposal/controller_test.gopkg/start/start.gotest/cvo/proposal.go
Comment on lines
+327
to
+335
| optr.proposalController = proposal.NewController(func() ([]configv1.Release, []configv1.ConditionalUpdate, error) { | ||
| availableUpdates := optr.getAvailableUpdates() | ||
| if availableUpdates == nil { | ||
| return nil, nil, nil | ||
| } | ||
| return availableUpdates.Updates, availableUpdates.ConditionalUpdates, nil | ||
| }, rtClient, cvInformer.Lister().Get, func(name, namespace string) (*corev1.ConfigMap, error) { | ||
| return cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name) | ||
| }, func() string { |
There was a problem hiding this comment.
Swap the ConfigMap callback parameters before prompt lookups break.
pkg/proposal/controller.go calls this getter as (namespace, name), but this callback interprets the arguments as (name, namespace) and then does ConfigMaps(namespace).Get(name). In practice that inverts the lookup and the proposal controller will fail to fetch its prompt ConfigMap.
🐛 Minimal fix
- }, rtClient, cvInformer.Lister().Get, func(name, namespace string) (*corev1.ConfigMap, error) {
- return cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name)
+ }, rtClient, cvInformer.Lister().Get, func(namespace, name string) (*corev1.ConfigMap, error) {
+ return cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name)
}, func() string {📝 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.
Suggested change
| optr.proposalController = proposal.NewController(func() ([]configv1.Release, []configv1.ConditionalUpdate, error) { | |
| availableUpdates := optr.getAvailableUpdates() | |
| if availableUpdates == nil { | |
| return nil, nil, nil | |
| } | |
| return availableUpdates.Updates, availableUpdates.ConditionalUpdates, nil | |
| }, rtClient, cvInformer.Lister().Get, func(name, namespace string) (*corev1.ConfigMap, error) { | |
| return cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name) | |
| }, func() string { | |
| optr.proposalController = proposal.NewController(func() ([]configv1.Release, []configv1.ConditionalUpdate, error) { | |
| availableUpdates := optr.getAvailableUpdates() | |
| if availableUpdates == nil { | |
| return nil, nil, nil | |
| } | |
| return availableUpdates.Updates, availableUpdates.ConditionalUpdates, nil | |
| }, rtClient, cvInformer.Lister().Get, func(namespace, name string) (*corev1.ConfigMap, error) { | |
| return cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name) | |
| }, func() string { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cvo/cvo.go` around lines 327 - 335, The ConfigMap getter passed into
proposal.NewController has its parameters reversed: the closure is declared as
func(name, namespace string) but proposal.Controller calls it as (namespace,
name), causing inverted lookups; fix by changing the closure signature to
func(namespace, name string) (*corev1.ConfigMap, error) and ensure it calls
cmConfigManagedInformer.Lister().ConfigMaps(namespace).Get(name) with the
corrected parameter order so optr.proposalController can fetch prompt ConfigMaps
correctly.
Comment on lines
+185
to
+191
| if optr.shouldEnableProposalController() { | ||
| if len(config.Status.History) < len(original.Status.History) { | ||
| klog.V(internal.Normal).Infof("Reconciling proposals because ClusterVersion.status.history got pruned") | ||
| // queue optr.proposalController.Sync() to manage proposals | ||
| optr.proposalController.Queue().Add(optr.proposalController.QueueKey()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential nil dereference when checking pruned history.
At Line 186, original.Status.History is accessed without guarding original. This can panic in nil-original paths that syncStatus otherwise supports.
💡 Suggested fix
if optr.shouldEnableProposalController() {
- if len(config.Status.History) < len(original.Status.History) {
+ originalHistoryLen := 0
+ if original != nil {
+ originalHistoryLen = len(original.Status.History)
+ }
+ if len(config.Status.History) < originalHistoryLen {
klog.V(internal.Normal).Infof("Reconciling proposals because ClusterVersion.status.history got pruned")
// queue optr.proposalController.Sync() to manage proposals
optr.proposalController.Queue().Add(optr.proposalController.QueueKey())
}
}📝 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.
Suggested change
| if optr.shouldEnableProposalController() { | |
| if len(config.Status.History) < len(original.Status.History) { | |
| klog.V(internal.Normal).Infof("Reconciling proposals because ClusterVersion.status.history got pruned") | |
| // queue optr.proposalController.Sync() to manage proposals | |
| optr.proposalController.Queue().Add(optr.proposalController.QueueKey()) | |
| } | |
| } | |
| if optr.shouldEnableProposalController() { | |
| originalHistoryLen := 0 | |
| if original != nil { | |
| originalHistoryLen = len(original.Status.History) | |
| } | |
| if len(config.Status.History) < originalHistoryLen { | |
| klog.V(internal.Normal).Infof("Reconciling proposals because ClusterVersion.status.history got pruned") | |
| // queue optr.proposalController.Sync() to manage proposals | |
| optr.proposalController.Queue().Add(optr.proposalController.QueueKey()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cvo/status.go` around lines 185 - 191, The code reads
original.Status.History without checking original (or original.Status), which
can panic; update the conditional in the block guarded by
optr.shouldEnableProposalController() to first ensure original is non-nil and
original.Status is non-nil (or otherwise treat original history length as 0)
before comparing lengths with config.Status.History, then only call
optr.proposalController.Queue().Add(...) when the safe length comparison
indicates pruning; reference optr.shouldEnableProposalController,
config.Status.History, original.Status.History, and
optr.proposalController.Queue().Add to locate and fix the check.
Comment on lines
+132
to
+145
| // OutputFieldItems defines the schema for array elements at the top level. | ||
| // Supports primitive types and objects (with nested OutputSubField properties). | ||
| type OutputFieldItems struct { | ||
| // type is the JSON type of array elements. | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Enum=string;number;boolean;object | ||
| Type OutputFieldType `json:"type"` | ||
|
|
||
| // properties defines fields for object-typed array elements. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| Properties []OutputSubField `json:"properties,omitempty"` | ||
| } |
There was a problem hiding this comment.
Enforce the object-item schema on OutputFieldItems.
OutputField validates that arrays must have items, but OutputFieldItems does not validate the next step: type: object can be created without properties, and primitive item types can still carry properties. That leaves the API accepting shapes the controller cannot interpret consistently.
As per coding guidelines, "When modifying code, check that nearby comments, kubernetes.io/description annotations, and doc strings still accurately describe the new behavior. Outdated documentation is worse than no documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/api/v1alpha1/agent_types.go` around lines 132 - 145,
OutputFieldItems currently allows Type=object without Properties and allows
Properties for primitive Types; add CRD validation rules on OutputFieldItems to
enforce that when Type (OutputFieldType) == "object" the Properties
([]OutputSubField) is required and non-empty, and when Type != "object"
Properties must be omitted/empty; implement this by adding kubebuilder
x-validation markers (XValidation) on the OutputFieldItems struct that check
Type == "object" implies Properties is present and has length > 0, and Type !=
"object" implies Properties is null/empty, so the controller will only receive
sensible object-item schemas (also update the struct comment for
OutputFieldItems to reflect the stricter validation).
Comment on lines
+197
to
+255
| // AgentSpec defines the desired state of Agent. | ||
| type AgentSpec struct { | ||
| // llmRef references a cluster-scoped LlmProvider CR that supplies the | ||
| // LLM backend for this agent. The operator resolves this reference at | ||
| // reconcile time and configures the sandbox pod with the provider's | ||
| // credentials and model. | ||
| // +kubebuilder:validation:Required | ||
| LLMRef corev1.LocalObjectReference `json:"llmRef"` | ||
|
|
||
| // skills defines one or more OCI images containing skills to mount | ||
| // in the agent's sandbox pod. Each entry specifies an image and optionally | ||
| // which paths within that image to mount. The operator creates Kubernetes | ||
| // image volumes (requires K8s 1.34+) and mounts them into the agent's | ||
| // skills directory. | ||
| // | ||
| // Multiple entries allow composing skills from different images: | ||
| // | ||
| // skills: | ||
| // - image: registry.ci.openshift.org/ocp/5.0:agentic-skills | ||
| // paths: | ||
| // - /skills/prometheus | ||
| // - /skills/cluster-update/update-advisor | ||
| // - image: quay.io/my-org/custom-skills:latest | ||
| // | ||
| // +kubebuilder:validation:MinItems=1 | ||
| Skills []SkillsSource `json:"skills"` | ||
|
|
||
| // mcpServers defines external MCP (Model Context Protocol) servers the | ||
| // agent can connect to for additional tools and context beyond its | ||
| // built-in skills. Each server is identified by name and URL. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| MCPServers []MCPServerConfig `json:"mcpServers,omitempty"` | ||
|
|
||
| // systemPromptRef references a ConfigMap containing the system prompt. | ||
| // The ConfigMap must have a key named "prompt" with the prompt text. | ||
| // The system prompt shapes the agent's behavior for its role (analysis, | ||
| // execution, or verification). When omitted, the agent uses a default | ||
| // prompt appropriate for its workflow step. | ||
| // +optional | ||
| SystemPromptRef *corev1.LocalObjectReference `json:"systemPromptRef,omitempty"` | ||
|
|
||
| // outputFields defines additional structured output fields beyond the | ||
| // base schema that every agent produces (diagnosis, proposal, RBAC, | ||
| // verification plan). Use this to request domain-specific structured | ||
| // data from the agent (e.g., an ACS violation ID, affected images). | ||
| // Mutually exclusive with rawOutputSchema. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| OutputFields []OutputField `json:"outputFields,omitempty"` | ||
|
|
||
| // rawOutputSchema is an escape hatch that replaces the entire output | ||
| // schema with a raw JSON Schema object. Use this when outputFields | ||
| // cannot express the schema you need (e.g., deeply nested structures, | ||
| // conditional fields). Mutually exclusive with outputFields. | ||
| // +optional | ||
| RawOutputSchema *apiextensionsv1.JSON `json:"rawOutputSchema,omitempty"` |
There was a problem hiding this comment.
Actually enforce outputFields vs rawOutputSchema exclusivity.
The field docs say these are mutually exclusive, but the CRD schema does not prevent both from being set. That makes the API contract ambiguous for clients and for any code that builds the final output schema.
As per coding guidelines, "When modifying code, check that nearby comments, kubernetes.io/description annotations, and doc strings still accurately describe the new behavior. Outdated documentation is worse than no documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/api/v1alpha1/agent_types.go` around lines 197 - 255, AgentSpec
documents that OutputFields and RawOutputSchema are mutually exclusive but the
CRD doesn't enforce it; implement validation in the API type by adding webhook
validation on the Agent (implement webhook.Validator methods ValidateCreate and
ValidateUpdate) that checks AgentSpec.OutputFields and AgentSpec.RawOutputSchema
and returns a validation error if both are non-empty; reference the AgentSpec
struct, the OutputFields slice and RawOutputSchema *apiextensionsv1.JSON, and
add identical checks in both ValidateCreate and ValidateUpdate to reject
requests that set both fields.
Comment on lines
+658
to
+677
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:printcolumn:name="Workflow",type=string,JSONPath=`.spec.workflowRef.name` | ||
| // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` | ||
| // +kubebuilder:printcolumn:name="Request",type=string,JSONPath=`.spec.request`,priority=1 | ||
| // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` | ||
|
|
||
| // Proposal represents a unit of work managed by the agentic platform. It is | ||
| // the final link in the CRD chain (LlmProvider -> Agent -> Workflow -> | ||
| // Proposal) and the primary resource users and adapters interact with. | ||
| // | ||
| // A Proposal references a Workflow that defines which agents handle each | ||
| // step, and tracks the full lifecycle from initial request through analysis, | ||
| // user approval, execution, and verification. Proposals are created by | ||
| // adapters (AlertManager webhook, ACS violation webhook, manual creation) | ||
| // or by the operator itself (escalation child proposals). | ||
| // | ||
| // Proposal is cluster-scoped. The operator watches for new Proposals and | ||
| // drives them through the lifecycle automatically. Users interact with | ||
| // proposals in the Proposed phase to approve, deny, or escalate. |
There was a problem hiding this comment.
Resolve the Proposal scope mismatch before publishing this API.
This comment says Proposal is cluster-scoped, but the type has no +kubebuilder:resource:scope=Cluster marker, and pkg/proposal/controller.go creates every Proposal with a namespace. Please align the CRD marker, controller behavior, and docs so clients are not told the wrong scope.
As per coding guidelines, "When modifying code, check that nearby comments, kubernetes.io/description annotations, and doc strings still accurately describe the new behavior. Outdated documentation is worse than no documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/api/v1alpha1/proposal_types.go` around lines 658 - 677, The
Proposal CRD documentation and behavior are inconsistent: the controller creates
Proposals with a namespace but the comment says "Proposal is cluster-scoped".
Update the Proposal type to declare the correct namespaced scope by adding the
kubebuilder marker +kubebuilder:resource:scope=Namespaced to the Proposal struct
and change the doc comment text from "Proposal is cluster-scoped" to reflect
that Proposals are namespaced; also verify pkg/proposal/controller.go still
creates Proposals with a namespace and adjust any creation logic only if you
intend to make Proposals cluster-scoped instead.
Comment on lines
+135
to
+138
| promptConfigMap, err := c.configMapGetterFunc(c.config.Namespace, c.config.PromptConfigMap) | ||
| if err != nil { | ||
| klog.V(i.Normal).Infof("Failed to get prompt ConfigMap %s/%s: %v", c.config.Namespace, c.config.PromptConfigMap, err) | ||
| return fmt.Errorf("failed to get prompt ConfigMap %s/%s: %w", c.config.Namespace, c.config.PromptConfigMap, err) |
There was a problem hiding this comment.
Swap the ConfigMap getter arguments.
configMapGetterFunc is (name, namespace), but this call passes (namespace, name). In practice that makes every prompt lookup query the wrong object and fail reconciliation.
Suggested fix
- promptConfigMap, err := c.configMapGetterFunc(c.config.Namespace, c.config.PromptConfigMap)
+ promptConfigMap, err := c.configMapGetterFunc(c.config.PromptConfigMap, c.config.Namespace)📝 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.
Suggested change
| promptConfigMap, err := c.configMapGetterFunc(c.config.Namespace, c.config.PromptConfigMap) | |
| if err != nil { | |
| klog.V(i.Normal).Infof("Failed to get prompt ConfigMap %s/%s: %v", c.config.Namespace, c.config.PromptConfigMap, err) | |
| return fmt.Errorf("failed to get prompt ConfigMap %s/%s: %w", c.config.Namespace, c.config.PromptConfigMap, err) | |
| promptConfigMap, err := c.configMapGetterFunc(c.config.PromptConfigMap, c.config.Namespace) | |
| if err != nil { | |
| klog.V(i.Normal).Infof("Failed to get prompt ConfigMap %s/%s: %v", c.config.Namespace, c.config.PromptConfigMap, err) | |
| return fmt.Errorf("failed to get prompt ConfigMap %s/%s: %w", c.config.Namespace, c.config.PromptConfigMap, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/controller.go` around lines 135 - 138, The call to
c.configMapGetterFunc in controller.go passes the arguments in the wrong order
(currently c.config.Namespace, c.config.PromptConfigMap) even though the
function signature is (name, namespace); change the call to pass the prompt
ConfigMap name first and the namespace second (use c.config.PromptConfigMap,
c.config.Namespace) so promptConfigMap, err := c.configMapGetterFunc(...)
queries the correct object and reconciliation succeeds.
Comment on lines
+157
to
+160
| proposals, err := getProposals(updates, conditionalUpdates, c.config.Namespace, currentVersion, cv.Spec.Channel, c.config.Workflow, prompt, readinessJSON) | ||
| if err != nil { | ||
| klog.V(i.Normal).Infof("Getting proposals hit an error: %v", err) | ||
| } |
There was a problem hiding this comment.
Propagate getProposals failures back to the reconciler.
This only logs the error. If proposal generation fails, Sync still returns success unless some unrelated path also failed, which suppresses retries and can silently leave the cluster without proposals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/controller.go` around lines 157 - 160, The getProposals error is
only logged in Sync, so failures are not propagated and prevent retries; update
the Sync call site to return the error instead of just logging it: when
getProposals (function getProposals) returns a non-nil err, wrap or annotate the
error with context (e.g., "getting proposals for channel X") and return it from
Sync so the reconciler sees the failure and can retry; remove or keep logging as
a debug/info entry but do not suppress propagation — ensure the caller of Sync
continues to receive the error.
Comment on lines
+187
to
+193
| if c.client.Create(ctx, proposal) != nil { | ||
| if !kerrors.IsAlreadyExists(err) { | ||
| klog.V(i.Normal).Infof("Failed to create proposal %s/%s: %v", proposal.Namespace, proposal.Name, err) | ||
| errs = append(errs, err) | ||
| } else { | ||
| klog.V(i.Debug).Infof("The proposal %s/%s existed already", proposal.Namespace, proposal.Name) | ||
| } |
There was a problem hiding this comment.
Capture the Create error instead of reading the stale outer err.
The result of c.client.Create(...) is discarded, so this branch is checking whatever err happened earlier. That hides real API failures and makes the AlreadyExists path unreliable.
Suggested fix
- if c.client.Create(ctx, proposal) != nil {
- if !kerrors.IsAlreadyExists(err) {
- klog.V(i.Normal).Infof("Failed to create proposal %s/%s: %v", proposal.Namespace, proposal.Name, err)
- errs = append(errs, err)
+ if err := c.client.Create(ctx, proposal); err != nil {
+ if !kerrors.IsAlreadyExists(err) {
+ klog.V(i.Normal).Infof("Failed to create proposal %s/%s: %v", proposal.Namespace, proposal.Name, err)
+ errs = append(errs, err)
} else {
klog.V(i.Debug).Infof("The proposal %s/%s existed already", proposal.Namespace, proposal.Name)
}
} else {📝 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.
Suggested change
| if c.client.Create(ctx, proposal) != nil { | |
| if !kerrors.IsAlreadyExists(err) { | |
| klog.V(i.Normal).Infof("Failed to create proposal %s/%s: %v", proposal.Namespace, proposal.Name, err) | |
| errs = append(errs, err) | |
| } else { | |
| klog.V(i.Debug).Infof("The proposal %s/%s existed already", proposal.Namespace, proposal.Name) | |
| } | |
| if err := c.client.Create(ctx, proposal); err != nil { | |
| if !kerrors.IsAlreadyExists(err) { | |
| klog.V(i.Normal).Infof("Failed to create proposal %s/%s: %v", proposal.Namespace, proposal.Name, err) | |
| errs = append(errs, err) | |
| } else { | |
| klog.V(i.Debug).Infof("The proposal %s/%s existed already", proposal.Namespace, proposal.Name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/controller.go` around lines 187 - 193, The call to
c.client.Create(ctx, proposal) discards its return error and the code
incorrectly checks the outer err; change it to capture the error (e.g. err :=
c.client.Create(ctx, proposal)) and then use that local err for the
kerrors.IsAlreadyExists check, the klog.V(...).Infof messages, and when
appending to errs so real API failures are logged and recorded; update all
references in this if/else branch that currently use the stale err to use the
newly captured err instead.
Comment on lines
+95
to
+97
| g.It("should create proposals", g.Label("Serial"), oteginkgo.Informing(), func() { | ||
| o.Expect(util.SkipIfNetworkRestricted(ctx, c, util.FauxinnatiAPIURL)).To(o.BeNil()) | ||
| util.SkipIfNotTechPreviewNoUpgrade(ctx, c) |
There was a problem hiding this comment.
Add the missing TechPreview labeling and Jira-formatted test name.
This case is gated with SkipIfNotTechPreviewNoUpgrade, but the It itself is not labeled as TechPreview and its name does not follow the required Jira-prefixed format.
As per coding guidelines, "Use Ginkgo Labels to mark test categories (e.g., TechPreview, serial)" and "Ensure test names follow the [Jira:"Cluster Version Operator"] description format".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cvo/proposal.go` around lines 95 - 97, The test case invoked via g.It
currently reads "should create proposals" and is gated by
util.SkipIfNotTechPreviewNoUpgrade but lacks the TechPreview label and the
Jira-prefixed name; update the g.It call so the test name follows the Jira
format (e.g., `[Jira:"Cluster Version Operator"] should create proposals`) and
add the TechPreview label in the label list (e.g., g.Label("Serial"),
g.Label("TechPreview")) while keeping oteginkgo.Informing() and the existing
util.SkipIfNetworkRestricted/util.SkipIfNotTechPreviewNoUpgrade checks intact.
Comment on lines
+110
to
+119
| g.By("Checking if the proposal are created") | ||
| o.Expect(wait.PollUntilContextTimeout(ctx, 30*time.Second, 5*time.Minute, true, func(ctx context.Context) (done bool, err error) { | ||
| proposals := proposalv1alpha1.ProposalList{} | ||
| err = rtClient.List(ctx, &proposals, ctrlruntimeclient.InNamespace(external.DefaultCVONamespace)) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if len(proposals.Items) == 0 { | ||
| return false, nil | ||
| } | ||
| return true, nil | ||
| })).NotTo(o.HaveOccurred(), "no proposals found") |
There was a problem hiding this comment.
Poll for newly created proposals, not just any existing Proposal.
This passes as soon as any Proposal exists in external.DefaultCVONamespace, so leftover CRs from a previous run can make the test green before this scenario creates anything. Capture the initial set/count or filter on the expected proposal identity before polling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cvo/proposal.go` around lines 110 - 119, The current poll returns true
as soon as any Proposal exists in external.DefaultCVONamespace; change this to
wait for the specific new Proposal(s) by first capturing the existing proposals
via rtClient.List(&proposalv1alpha1.ProposalList{}) to record the initial
set/count, then use wait.PollUntilContextTimeout to either (a) check that the
count has increased beyond the recorded initial count, or (b) list and filter
for the expected proposal identity (by name or label) and return true only when
that specific Proposal is present; update the polling closure that currently
calls rtClient.List(...) against external.DefaultCVONamespace accordingly to
compare against the initial snapshot or to match the expected name/label.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Will rebase after #1381 gets in.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests