OTA-1966: Init the Proprosal Lifecycle Controller#1381
Open
hongkailiu wants to merge 7 commits intoopenshift:mainfrom
Open
OTA-1966: Init the Proprosal Lifecycle Controller#1381hongkailiu wants to merge 7 commits intoopenshift:mainfrom
hongkailiu wants to merge 7 commits intoopenshift:mainfrom
Conversation
Contributor
|
@hongkailiu: This pull request references OTA-1966 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughAdds a proposal controller and extensive new CRD schemas (Proposal, Workflow, Agent, LlmProvider, OLSConfig); integrates the controller into the Operator lifecycle and enqueues proposal syncs from available-updates; controller reads available-updates and creates Proposal objects in the Kubernetes API. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator
participant Queue as ProposalQueue
participant Controller as ProposalController
participant Updates as AvailableUpdatesProvider
participant K8s as Kubernetes API
Operator->>Queue: add(optr.proposalController.QueueKey())
Note right of Queue: key enqueued
loop worker
Queue->>Controller: pop key -> Sync(ctx,key)
Controller->>Updates: UpdatesGetterFunc() -> releases, conditional updates
Updates-->>Controller: releases, conditional updates
Controller->>Controller: compute proposals
Controller->>K8s: Create Proposal objects
K8s-->>Controller: Created / AlreadyExists / Error
Controller-->>Queue: return (error => requeue) or success
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 |
openshift-ci
Bot
added
the
approved
hongkailiu
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![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 (3)
pkg/proposal/controller.go (2)
58-63: Avoid dumping the full release list at info level.
%#von the whole[]configv1.Releaseslice will get noisy quickly once this controller starts reconciling for real. Logging a count, or a compact version list behindV(2), keeps the signal without ballooning normal logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 58 - 63, Replace the noisy full-slice info log: instead of klog.Infof("Got available updates: %#v", updates) log the count at info-level and move the full/compact list to a verbose level; e.g., use klog.Infof("Got %d available updates", len(updates)) and klog.V(2).Infof("Available updates: %#v", updates) or build a small slice of release.Version strings and log that at V(2); update the code around availableUpdatesGetterFunc, updates and the klog calls accordingly.
18-21: Update the configuration-oriented comments and log messages.This file is the proposal lifecycle controller, but the queue comment and
Sync()logs still say “CVO configuration”. That will send operators to the wrong subsystem when debugging. Please rename them to match proposal lifecycle behavior and replace the placeholderSync ensures:comment with the actual contract. 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."Also applies to: 49-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 18 - 21, Update the outdated "CVO configuration" comments and log messages to refer to the proposal lifecycle controller and proposal lifecycle behavior: change the field comment for queue (workqueue.TypedRateLimitingInterface[any]) to describe that it tracks proposal lifecycle reconciliation tasks, update any Sync() log messages and their logger context to mention "proposal lifecycle" rather than "CVO configuration", and replace the placeholder "Sync ensures:" comment with the actual contract of Sync (state explicitly what invariants Sync enforces, inputs/outputs, and when it requeues or returns errors). Also search nearby comments and kubernetes.io/description annotations in this file (including the block around lines 49–56) and update them consistently to reflect the proposal lifecycle controller semantics.pkg/cvo/availableupdates.go (1)
185-186: Gate proposal scheduling with the same enablement check.
Operator.Run()only starts the proposal worker behindshouldEnableProposalController(), but this path always enqueues proposal work. If that switch is ever turned off, the controller is still being scheduled even though nothing drains its queue. Please guard thisAdd()behind the same helper or move proposal scheduling behind a shared method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/availableupdates.go` around lines 185 - 186, The proposal queue is always enqueued even when proposals are disabled; wrap the enqueue call so it only runs when shouldEnableProposalController() is true (or move both worker start and enqueue into a single helper). Specifically, guard the call to optr.proposalController.Queue().Add(optr.proposalController.QueueKey()) with the same shouldEnableProposalController() check used when starting the proposal worker (or refactor into a shared method that starts the worker and schedules the initial Add together).
🤖 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 324-329: The closure passed to optr.proposalController is reading
the shared optr.availableUpdates without holding Operator.statusLock, causing a
data race; modify the closure to call the existing locked accessor (or add one)
that grabs statusLock (RLock), reads/copies the Updates slice, and returns that
copy (do not return the internal slice pointer). Use the Operator accessor
rather than accessing optr.availableUpdates directly and ensure readers use the
same lock discipline as syncAvailableUpdates()/setAvailableUpdates().
---
Nitpick comments:
In `@pkg/cvo/availableupdates.go`:
- Around line 185-186: The proposal queue is always enqueued even when proposals
are disabled; wrap the enqueue call so it only runs when
shouldEnableProposalController() is true (or move both worker start and enqueue
into a single helper). Specifically, guard the call to
optr.proposalController.Queue().Add(optr.proposalController.QueueKey()) with the
same shouldEnableProposalController() check used when starting the proposal
worker (or refactor into a shared method that starts the worker and schedules
the initial Add together).
In `@pkg/proposal/controller.go`:
- Around line 58-63: Replace the noisy full-slice info log: instead of
klog.Infof("Got available updates: %#v", updates) log the count at info-level
and move the full/compact list to a verbose level; e.g., use klog.Infof("Got %d
available updates", len(updates)) and klog.V(2).Infof("Available updates: %#v",
updates) or build a small slice of release.Version strings and log that at V(2);
update the code around availableUpdatesGetterFunc, updates and the klog calls
accordingly.
- Around line 18-21: Update the outdated "CVO configuration" comments and log
messages to refer to the proposal lifecycle controller and proposal lifecycle
behavior: change the field comment for queue
(workqueue.TypedRateLimitingInterface[any]) to describe that it tracks proposal
lifecycle reconciliation tasks, update any Sync() log messages and their logger
context to mention "proposal lifecycle" rather than "CVO configuration", and
replace the placeholder "Sync ensures:" comment with the actual contract of Sync
(state explicitly what invariants Sync enforces, inputs/outputs, and when it
requeues or returns errors). Also search nearby comments and
kubernetes.io/description annotations in this file (including the block around
lines 49–56) and update them consistently to reflect the proposal lifecycle
controller semantics.
🪄 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: Pro Plus
Run ID: be2ee926-1545-4d17-9b6b-0ecd5918efaf
📒 Files selected for processing (3)
pkg/cvo/availableupdates.gopkg/cvo/cvo.gopkg/proposal/controller.go
wking
reviewed
Apr 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/cvo/cvo.go (2)
331-335:⚠️ Potential issue | 🔴 CriticalDon't read
optr.availableUpdatesdirectly from the proposal worker.This closure runs on the proposal worker goroutine without
statusLock, and it returns the internal slices directly.syncAvailableUpdates()can replace/sort that state concurrently, so this introduces a real data race and exposes partially updated data.Suggested fix
optr.proposalController = proposal.NewController(func() ([]configv1.Release, []configv1.ConditionalUpdate, error) { - if optr.availableUpdates == nil { + availableUpdates := optr.getAvailableUpdates() + if availableUpdates == nil { return nil, nil, nil } - return optr.availableUpdates.Updates, optr.availableUpdates.ConditionalUpdates, nil + return append([]configv1.Release(nil), availableUpdates.Updates...), + append([]configv1.ConditionalUpdate(nil), availableUpdates.ConditionalUpdates...), + nil }, c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/cvo.go` around lines 331 - 335, The closure passed to proposal.NewController reads optr.availableUpdates without holding the optr.statusLock, causing a data race; modify the closure in optr.proposalController so it acquires optr.statusLock (or otherwise synchronizes with syncAvailableUpdates()), checks for nil, and returns deep copies of the slices (make new slices and copy elements from optr.availableUpdates.Updates and .ConditionalUpdates) instead of returning the internal slices directly; this ensures the proposal worker never accesses partially-updated shared state.
547-558:⚠️ Potential issue | 🟠 MajorDon't hard-enable the proposal worker yet.
shouldEnableProposalController()always returns true, so every CVO instance starts this controller even when the Proposal API/CRDs or feature-gate prerequisites are absent. In those clusters this worker will just spin on failing creates and add noisy retries.Also applies to: 1228-1231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/cvo.go` around lines 547 - 558, The current startup unconditionally launches the proposal controller because shouldEnableProposalController() always returns true; change the guard so the controller only starts after verifying the Proposal API/CRDs and feature gate are present. Replace the current conditional that checks shouldEnableProposalController() (and the identical checks at the other location) with a call that verifies API availability and the feature gate (for example implement optr.canStartProposalController() which uses the API discovery/RESTMapper or ServerResourcesForGroupVersion to confirm the Proposal CRD/group-version exists and checks the feature gate via utilfeature.DefaultFeatureGate.Enabled), and only then start the goroutine that calls optr.worker(..., optr.proposalController.Queue(), optr.proposalController.Sync); otherwise log that the proposal controller is disabled. Ensure the new check is used in both places mentioned.
🤖 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-330: The constructor currently calls config.GetConfigOrDie()
which exits the process on failure and violates this function's error-return
contract; change the code to obtain a kubeconfig that returns (cfg, err) (e.g.
use a non-fatal API such as config.GetConfig() or
clientcmd.BuildConfigFromFlags()/rest.InClusterConfig wrapper), check and return
the error if present, then pass the cfg into runtimeclient.New(cfg,
runtimeclient.Options{}); ensure you remove the GetConfigOrDie() call so New()'s
error can be propagated to the caller.
In `@pkg/proposal/controller.go`:
- Around line 92-106: getProposals currently ignores its inputs and always
returns one hard-coded Proposal; change it to iterate over the updates and
conditionalUpdates parameters and construct a Proposal for each real update (or
a single Proposal that encodes the update set) instead of the fixed-name item.
Use the updates and conditionalUpdates slices to populate Proposal.Spec.Request,
WorkflowRef and metadata (avoid using controllerName as the constant Name);
reference the getProposals function, the updates and conditionalUpdates
parameters, and proposalv1alpha1.Proposal/ProposalSpec to build appropriately
named/unique Proposals (or a single Proposal whose Spec contains the serialized
update list) so the emitted resources reflect the actual update data passed in.
- Around line 78-88: The loop in the controller that calls c.client.Create for
each item in proposals must reconcile existing resources instead of ignoring
kapierrors.IsAlreadyExists: for each proposal returned by the reconciler
(variable proposals and loop), attempt to Get the existing object (using the
same name/namespace/UID), if not found fall back to Create, but if found compare
the desired spec/labels/annotations and call c.client.Update or Patch to apply
changes; additionally detect proposals present in cluster but absent from the
desired proposals list and delete them via c.client.Delete; aggregate and return
any Create/Get/Update/Delete errors (replace the current IsAlreadyExists drop
logic around c.client.Create with this reconcile flow referencing
c.client.Create, c.client.Get, c.client.Update, and c.client.Delete).
- Around line 25-27: Update all copied comments, log messages, and
kubernetes.io/description annotations that refer to "CVO configuration" to
correctly refer to the proposal controller behavior (e.g., "proposal
controller", "proposal configuration", or similar) so they match the actual
code; specifically edit the comment above the queue variable that currently
mentions "CVO configuration" and any other comments/log lines around the queue
and the cvo.Operator worker method (the type any usage) as well as the doc
strings referenced near the same block (previously lines noted around 59-65) to
use accurate wording. Ensure phrasing is consistent across comments, logs, and
annotations and preserves the note about using type any to satisfy
cvo.Operator's worker signature.
---
Duplicate comments:
In `@pkg/cvo/cvo.go`:
- Around line 331-335: The closure passed to proposal.NewController reads
optr.availableUpdates without holding the optr.statusLock, causing a data race;
modify the closure in optr.proposalController so it acquires optr.statusLock (or
otherwise synchronizes with syncAvailableUpdates()), checks for nil, and returns
deep copies of the slices (make new slices and copy elements from
optr.availableUpdates.Updates and .ConditionalUpdates) instead of returning the
internal slices directly; this ensures the proposal worker never accesses
partially-updated shared state.
- Around line 547-558: The current startup unconditionally launches the proposal
controller because shouldEnableProposalController() always returns true; change
the guard so the controller only starts after verifying the Proposal API/CRDs
and feature gate are present. Replace the current conditional that checks
shouldEnableProposalController() (and the identical checks at the other
location) with a call that verifies API availability and the feature gate (for
example implement optr.canStartProposalController() which uses the API
discovery/RESTMapper or ServerResourcesForGroupVersion to confirm the Proposal
CRD/group-version exists and checks the feature gate via
utilfeature.DefaultFeatureGate.Enabled), and only then start the goroutine that
calls optr.worker(..., optr.proposalController.Queue(),
optr.proposalController.Sync); otherwise log that the proposal controller is
disabled. Ensure the new check is used in both places mentioned.
🪄 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: Pro Plus
Run ID: 2cbd6bae-c859-4116-ac7b-579b5dbc890a
📒 Files selected for processing (3)
pkg/cvo/availableupdates.gopkg/cvo/cvo.gopkg/proposal/controller.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cvo/availableupdates.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/proposal/api/v1alpha1/olsconfig_types.go (2)
157-158: Document the shared type reference.The comment in
agent_types.go(lines 157-158) references thatMCPServerConfigand related types are defined here. Consider adding a reciprocal comment here noting these types are shared with the Agent API, to help maintainers understand the coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/api/v1alpha1/olsconfig_types.go` around lines 157 - 158, Add a reciprocal comment above the shared types in olsconfig_types.go indicating that MCPServerConfig (and any related types) are shared with the Agent API (referenced from agent_types.go) so maintainers understand this coupling; place the note near the LogLevel comment block and explicitly name MCPServerConfig and the related types to make the cross-file dependency discoverable.
1-2: Inconsistent copyright year.This file uses "Copyright 2024" while other new files in this PR use "Copyright 2026". Consider updating for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/api/v1alpha1/olsconfig_types.go` around lines 1 - 2, Update the top-of-file copyright year in pkg/proposal/api/v1alpha1/olsconfig_types.go to match the rest of the PR (change "Copyright 2024" to "Copyright 2026") by editing the file header comment at the very top of the file.pkg/proposal/api/v1alpha1/proposal_types.go (1)
175-193: Consider makingActionsfield explicitly required or optional.
ProposalResult.Actions(line 180) lacks both+kubebuilder:validation:Requiredand+optionalmarkers. If an empty actions list is invalid, add aMinItems=1validation. If it can be empty, add+optionalfor clarity.🤖 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 175 - 193, ProposalResult.Actions is missing explicit kubebuilder validation; decide whether the Actions slice must be non-empty or may be omitted and update ProposalResult accordingly: if an empty actions list is invalid add a kubebuilder validation tag like `+kubebuilder:validation:MinItems=1` on the Actions field (ProposedAction/Actions) so the API enforces at least one item, otherwise mark the field optional by adding `+optional` to clarify it may be omitted or empty.pkg/proposal/api/v1alpha1/llmprovider_types.go (1)
58-63: Clarify namespace resolution forLocalObjectReferenceon cluster-scoped resource.
corev1.LocalObjectReferencetypically references objects in the same namespace. SinceLlmProvideris cluster-scoped, the documentation should clarify which namespace the operator looks for the secret in (e.g., operator's namespace, or a well-known namespace). Consider updating the comment at line 58-61 to specify the expected namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/api/v1alpha1/llmprovider_types.go` around lines 58 - 63, The comment for the CredentialsSecretRef field is ambiguous about namespace resolution for corev1.LocalObjectReference on the cluster-scoped LlmProvider; update the field comment (around CredentialsSecretRef in llmprovider_types.go) to explicitly state which namespace the operator will look up the Secret in (for example: "operator's namespace" or a specific well-known namespace), and mention that the reference is expected to point to a Secret in that namespace so callers know where to place credentials and how they will be injected at runtime.
🤖 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/proposal/api/v1alpha1/agent_types.go`:
- Around line 240-256: Add a kubebuilder XValidation marker to enforce the
documented mutual exclusion between OutputFields and RawOutputSchema: add a
validation rule on the Agent or AgentSpec type that forbids having both
spec.outputFields (non-empty) and spec.rawOutputSchema set at the same time,
using an XValidation rule like the suggested negated has/size expression and a
message "outputFields and rawOutputSchema are mutually exclusive"; target the
type where OutputFields []OutputField and RawOutputSchema *apiextensionsv1.JSON
are declared so the CRD prevents admission when both are provided.
---
Nitpick comments:
In `@pkg/proposal/api/v1alpha1/llmprovider_types.go`:
- Around line 58-63: The comment for the CredentialsSecretRef field is ambiguous
about namespace resolution for corev1.LocalObjectReference on the cluster-scoped
LlmProvider; update the field comment (around CredentialsSecretRef in
llmprovider_types.go) to explicitly state which namespace the operator will look
up the Secret in (for example: "operator's namespace" or a specific well-known
namespace), and mention that the reference is expected to point to a Secret in
that namespace so callers know where to place credentials and how they will be
injected at runtime.
In `@pkg/proposal/api/v1alpha1/olsconfig_types.go`:
- Around line 157-158: Add a reciprocal comment above the shared types in
olsconfig_types.go indicating that MCPServerConfig (and any related types) are
shared with the Agent API (referenced from agent_types.go) so maintainers
understand this coupling; place the note near the LogLevel comment block and
explicitly name MCPServerConfig and the related types to make the cross-file
dependency discoverable.
- Around line 1-2: Update the top-of-file copyright year in
pkg/proposal/api/v1alpha1/olsconfig_types.go to match the rest of the PR (change
"Copyright 2024" to "Copyright 2026") by editing the file header comment at the
very top of the file.
In `@pkg/proposal/api/v1alpha1/proposal_types.go`:
- Around line 175-193: ProposalResult.Actions is missing explicit kubebuilder
validation; decide whether the Actions slice must be non-empty or may be omitted
and update ProposalResult accordingly: if an empty actions list is invalid add a
kubebuilder validation tag like `+kubebuilder:validation:MinItems=1` on the
Actions field (ProposedAction/Actions) so the API enforces at least one item,
otherwise mark the field optional by adding `+optional` to clarify it may be
omitted or empty.
🪄 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: Pro Plus
Run ID: c2f541c2-528f-4d4b-9c6f-165464e20294
⛔ Files ignored due to path filters (47)
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/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/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/log/log.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 (8)
go.modpkg/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.go
Comment on lines
+240
to
+256
| // 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.
Missing XValidation for mutual exclusion between outputFields and rawOutputSchema.
The documentation states these fields are mutually exclusive (lines 244, 253), but there's no XValidation rule to enforce this at admission time. Users could set both, leading to undefined behavior.
🛠️ Suggested fix - Add XValidation to AgentSpec or Agent
Add this validation marker to the Agent type or AgentSpec:
// +kubebuilder:validation:XValidation:rule="!(has(self.spec.outputFields) && size(self.spec.outputFields) > 0 && has(self.spec.rawOutputSchema))",message="outputFields and rawOutputSchema are mutually exclusive"Or on AgentSpec:
// +kubebuilder:validation:XValidation:rule="!(has(self.outputFields) && size(self.outputFields) > 0 && has(self.rawOutputSchema))",message="outputFields and rawOutputSchema are mutually exclusive"
type AgentSpec struct {🤖 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 240 - 256, Add a
kubebuilder XValidation marker to enforce the documented mutual exclusion
between OutputFields and RawOutputSchema: add a validation rule on the Agent or
AgentSpec type that forbids having both spec.outputFields (non-empty) and
spec.rawOutputSchema set at the same time, using an XValidation rule like the
suggested negated has/size expression and a message "outputFields and
rawOutputSchema are mutually exclusive"; target the type where OutputFields
[]OutputField and RawOutputSchema *apiextensionsv1.JSON are declared so the CRD
prevents admission when both are provided.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
pkg/proposal/controller.go (4)
59-66:⚠️ Potential issue | 🟡 MinorLog messages reference "CVO configuration" instead of proposal controller.
The log messages at lines 63 and 65 say "syncing CVO configuration" but this is the proposal controller. Update these to reflect the actual purpose (e.g., "proposal sync" or similar).
As per coding guidelines, comments and logs should accurately describe the behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 59 - 66, The log messages in Controller.Sync incorrectly mention "CVO configuration"; update the two klog.V(i.Normal).Infof calls inside the Sync method (the start log and the deferred finish log that reference startTime) to accurately describe this as the proposal controller (e.g., "Started proposal sync %q" and "Finished proposal sync (%v)"). Locate the Sync function on type Controller and replace the message strings passed to Infof while keeping the same formatting arguments.
92-108:⚠️ Potential issue | 🟠 MajorPlaceholder implementation does not use update data.
The
getProposalsfunction ignores itsupdatesandconditionalUpdatesparameters and returns a single hardcoded Proposal. The TODO comment acknowledges this, but the controller cannot represent actual available updates until this is implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 92 - 108, getProposals currently ignores its parameters (updates, conditionalUpdates) and returns a single hardcoded proposal; update it to build and return Proposal objects based on the provided updates and conditionalUpdates so the controller represents real available updates. Specifically, iterate over the updates and/or conditionalUpdates in getProposals, populate ProposalSpec fields (e.g., Request, WorkflowRef, and any metadata) using values from each configv1.Release/configv1.ConditionalUpdate, create one proposal per relevant update (or aggregate into meaningful proposals as required), and return the slice of *proposalv1alpha1.Proposal instead of the static literal; ensure you preserve required fields (ObjectMeta.Name/Namespace) and set WorkflowRef appropriately rather than using the hardcoded "ota-advisory".
78-88:⚠️ Potential issue | 🟠 MajorReconcile existing proposals instead of dropping
AlreadyExists.The current implementation only creates proposals and ignores
AlreadyExistserrors. This is write-once behavior — changes to the desired set of proposals are never reconciled. A lifecycle controller typically needs Get/Update/Delete logic to converge state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 78 - 88, The loop in pkg/proposal/controller.go currently calls c.client.Create for each proposal and ignores AlreadyExists errors (using kapierrors.IsAlreadyExists), which prevents reconciling updates; change the logic in the proposals iteration to attempt a Get (c.client.Get) when Create returns AlreadyExists, compare the existing object with the desired proposal, and call Update (c.client.Update) when they differ (or Delete when the proposal should be removed); ensure you still aggregate non-AlreadyExists errors into errs and return kutilerrors.NewAggregate(errs), and reference the existing symbols c.client.Create, kapierrors.IsAlreadyExists, c.client.Get, and c.client.Update to implement the reconcile path.
25-27:⚠️ Potential issue | 🟡 MinorRename the copied CVO-configuration docs and logs.
The comment still references "CVO configuration" but this is the proposal controller. This creates confusion during debugging and maintenance.
As per coding guidelines, "When modifying code, check that nearby comments, kubernetes.io/description annotations, and doc strings still accurately describe the new behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` around lines 25 - 27, Update the misleading "CVO configuration" references in pkg/proposal/controller.go: change the comment above the queue variable and any nearby log messages, kubernetes.io/description annotations, and docstrings that mention "CVO configuration" to accurately say "proposal controller" (or the specific proposal responsibility) so they reflect the actual behavior of the Proposal controller; search for the symbol queue and any functions/methods in this file (e.g., the controller struct and its worker methods) and update all surrounding comments and logs consistently.pkg/proposal/api/v1alpha1/agent_types.go (1)
240-256:⚠️ Potential issue | 🟡 MinorMissing XValidation for mutual exclusion between
outputFieldsandrawOutputSchema.The documentation states these fields are mutually exclusive (lines 244, 253), but there's no XValidation rule to enforce this at admission time. Users could set both, leading to undefined behavior.
🤖 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 240 - 256, Add a kubebuilder XValidation rule to enforce that OutputFields and RawOutputSchema are mutually exclusive by adding a marker like +kubebuilder:validation:XValidation:rule="self.rawOutputSchema == null || size(self.outputFields) == 0",message="only one of outputFields or rawOutputSchema may be set" to the Agent spec struct (near the definitions of OutputFields and RawOutputSchema) so admission rejects objects that set both OutputFields and RawOutputSchema.
🧹 Nitpick comments (2)
pkg/proposal/api/v1alpha1/proposal_types.go (1)
224-234: Consider adding+listType=mapwith+listMapKey=nametoVerificationPlan.Steps.
VerificationStephas aNamefield that appears to be a unique identifier. Adding list type annotations would enable strategic merge patch semantics and ensure stable ordering in the OpenAPI schema, consistent with howPropertiesis annotated inOutputField(line 94-96 of agent_types.go).♻️ Suggested annotation
type VerificationPlan struct { // description is a human-readable summary of the verification approach. Description string `json:"description"` // steps is the ordered list of verification checks to run. // +optional + // +listType=map + // +listMapKey=name Steps []VerificationStep `json:"steps,omitempty"`🤖 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 224 - 234, The Steps slice on VerificationPlan should be annotated as a map keyed by each step's Name to enable stable ordering and strategic merge patch behavior; update the VerificationPlan definition to add the Kubernetes markers "+listType=map" and "+listMapKey=name" above the Steps field (reference VerificationPlan.Steps and the unique identifier field VerificationStep.Name) similar to how OutputField.Properties is annotated; ensure the tag placement follows other CRD markers so generated OpenAPI schema and strategic merge patches treat Steps as a keyed map.pkg/proposal/controller.go (1)
97-97: Consider using empty string directly instead ofmetav1.NamespaceAll.
metav1.NamespaceAllis semantically meant for list operations ("list across all namespaces"). For a cluster-scoped resource whereNamespaceshould be empty, consider using""directly or simply omitting the field to avoid confusion.♻️ Suggested clarification
{ ObjectMeta: metav1.ObjectMeta{ Name: controllerName, - Namespace: metav1.NamespaceAll, + // Namespace is empty for cluster-scoped resources },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/proposal/controller.go` at line 97, Replace the use of metav1.NamespaceAll in the object literal (the line "Namespace: metav1.NamespaceAll") with an empty string or omit the Namespace field entirely for this cluster-scoped resource; update the struct/variable where "Namespace: metav1.NamespaceAll" appears so it uses "" (or drops the Namespace key) to avoid the misleading list-scoped semantic of metav1.NamespaceAll.
🤖 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/proposal/api/v1alpha1/agent_types.go`:
- Around line 99-130: OutputSubField currently allows Type values from
OutputFieldType (including "object") but doesn't provide a Properties field to
define object schemas; update the type definition to prevent invalid state by
either adding a Properties map field (e.g., Properties map[string]OutputSubField
`json:"properties,omitempty"`) to OutputSubField and document/use it when Type
== "object", or create a new enum/type OutputSubFieldType (or a constrained
validation) that excludes "object" and replace OutputSubField.Type with that new
type; refer to OutputSubField, OutputFieldType, and Properties when making the
change so consumers can't declare type: "object" without a schema.
In `@pkg/proposal/api/v1alpha1/olsconfig_types.go`:
- Line 2: Update the copyright header year in the top-of-file comment from 2024
to 2026 to match the other generated types (e.g., the headers used in
llmprovider_types.go, agent_types.go, proposal_types.go); locate the file's
header comment (the package-level/top comment in olsconfig_types.go) and change
the year value to 2026 for consistency.
---
Duplicate comments:
In `@pkg/proposal/api/v1alpha1/agent_types.go`:
- Around line 240-256: Add a kubebuilder XValidation rule to enforce that
OutputFields and RawOutputSchema are mutually exclusive by adding a marker like
+kubebuilder:validation:XValidation:rule="self.rawOutputSchema == null ||
size(self.outputFields) == 0",message="only one of outputFields or
rawOutputSchema may be set" to the Agent spec struct (near the definitions of
OutputFields and RawOutputSchema) so admission rejects objects that set both
OutputFields and RawOutputSchema.
In `@pkg/proposal/controller.go`:
- Around line 59-66: The log messages in Controller.Sync incorrectly mention
"CVO configuration"; update the two klog.V(i.Normal).Infof calls inside the Sync
method (the start log and the deferred finish log that reference startTime) to
accurately describe this as the proposal controller (e.g., "Started proposal
sync %q" and "Finished proposal sync (%v)"). Locate the Sync function on type
Controller and replace the message strings passed to Infof while keeping the
same formatting arguments.
- Around line 92-108: getProposals currently ignores its parameters (updates,
conditionalUpdates) and returns a single hardcoded proposal; update it to build
and return Proposal objects based on the provided updates and conditionalUpdates
so the controller represents real available updates. Specifically, iterate over
the updates and/or conditionalUpdates in getProposals, populate ProposalSpec
fields (e.g., Request, WorkflowRef, and any metadata) using values from each
configv1.Release/configv1.ConditionalUpdate, create one proposal per relevant
update (or aggregate into meaningful proposals as required), and return the
slice of *proposalv1alpha1.Proposal instead of the static literal; ensure you
preserve required fields (ObjectMeta.Name/Namespace) and set WorkflowRef
appropriately rather than using the hardcoded "ota-advisory".
- Around line 78-88: The loop in pkg/proposal/controller.go currently calls
c.client.Create for each proposal and ignores AlreadyExists errors (using
kapierrors.IsAlreadyExists), which prevents reconciling updates; change the
logic in the proposals iteration to attempt a Get (c.client.Get) when Create
returns AlreadyExists, compare the existing object with the desired proposal,
and call Update (c.client.Update) when they differ (or Delete when the proposal
should be removed); ensure you still aggregate non-AlreadyExists errors into
errs and return kutilerrors.NewAggregate(errs), and reference the existing
symbols c.client.Create, kapierrors.IsAlreadyExists, c.client.Get, and
c.client.Update to implement the reconcile path.
- Around line 25-27: Update the misleading "CVO configuration" references in
pkg/proposal/controller.go: change the comment above the queue variable and any
nearby log messages, kubernetes.io/description annotations, and docstrings that
mention "CVO configuration" to accurately say "proposal controller" (or the
specific proposal responsibility) so they reflect the actual behavior of the
Proposal controller; search for the symbol queue and any functions/methods in
this file (e.g., the controller struct and its worker methods) and update all
surrounding comments and logs consistently.
---
Nitpick comments:
In `@pkg/proposal/api/v1alpha1/proposal_types.go`:
- Around line 224-234: The Steps slice on VerificationPlan should be annotated
as a map keyed by each step's Name to enable stable ordering and strategic merge
patch behavior; update the VerificationPlan definition to add the Kubernetes
markers "+listType=map" and "+listMapKey=name" above the Steps field (reference
VerificationPlan.Steps and the unique identifier field VerificationStep.Name)
similar to how OutputField.Properties is annotated; ensure the tag placement
follows other CRD markers so generated OpenAPI schema and strategic merge
patches treat Steps as a keyed map.
In `@pkg/proposal/controller.go`:
- Line 97: Replace the use of metav1.NamespaceAll in the object literal (the
line "Namespace: metav1.NamespaceAll") with an empty string or omit the
Namespace field entirely for this cluster-scoped resource; update the
struct/variable where "Namespace: metav1.NamespaceAll" appears so it uses "" (or
drops the Namespace key) to avoid the misleading list-scoped semantic of
metav1.NamespaceAll.
🪄 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: Pro Plus
Run ID: 14ae3070-e8d6-43c4-81b5-3f8be460afd7
⛔ Files ignored due to path filters (47)
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/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/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/log/log.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 (12)
go.modpkg/cvo/availableupdates.gopkg/cvo/cvo.gopkg/featuregates/featuregates.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.go
✅ Files skipped from review due to trivial changes (1)
- pkg/proposal/api/v1alpha1/groupversion_info.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cvo/availableupdates.go
- go.mod
- pkg/cvo/cvo.go
- pkg/proposal/api/v1alpha1/workflow_types.go
Comment on lines
+99
to
+130
| // OutputSubField defines a nested field (one level deep) within an OutputField | ||
| // of type "object" or within array items of type "object". At this depth, | ||
| // array items are restricted to primitive types (string, number, boolean). | ||
| // +kubebuilder:validation:XValidation:rule="self.type == 'array' ? has(self.items) : true",message="items is required when type is array" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.enum) ? self.type == 'string' : true",message="enum is only valid for string fields" | ||
| type OutputSubField struct { | ||
| // name is the field name in the output JSON. | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z][a-zA-Z0-9_]*$" | ||
| Name string `json:"name"` | ||
|
|
||
| // type is the JSON type of this field. | ||
| // +kubebuilder:validation:Required | ||
| Type OutputFieldType `json:"type"` | ||
|
|
||
| // description explains the purpose of this field (passed to the LLM). | ||
| // +optional | ||
| Description string `json:"description,omitempty"` | ||
|
|
||
| // required indicates whether the agent must populate this field. | ||
| // +optional | ||
| Required bool `json:"required,omitempty"` | ||
|
|
||
| // enum constrains string fields to a set of allowed values. | ||
| // +optional | ||
| Enum []string `json:"enum,omitempty"` | ||
|
|
||
| // items defines the element schema when type is array (primitive types only at this depth). | ||
| // +optional | ||
| Items *OutputSubFieldItems `json:"items,omitempty"` | ||
| } |
There was a problem hiding this comment.
OutputSubField allows type: object but has no properties field.
OutputSubField.Type uses OutputFieldType which includes object, but unlike OutputField, OutputSubField does not have a Properties field. This means users can set type: object on a sub-field but cannot define its schema.
Either:
- Add a
Propertiesfield toOutputSubField, or - Restrict
OutputSubField.Typeto excludeobjectvia a dedicated enum
🛠️ Option 2: Restrict the enum
type OutputSubField struct {
// ...
// type is the JSON type of this field.
// +kubebuilder:validation:Required
+ // +kubebuilder:validation:Enum=string;number;boolean;array
Type OutputFieldType `json:"type"`Or define a separate OutputSubFieldType enum that excludes object.
🤖 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 99 - 130,
OutputSubField currently allows Type values from OutputFieldType (including
"object") but doesn't provide a Properties field to define object schemas;
update the type definition to prevent invalid state by either adding a
Properties map field (e.g., Properties map[string]OutputSubField
`json:"properties,omitempty"`) to OutputSubField and document/use it when Type
== "object", or create a new enum/type OutputSubFieldType (or a constrained
validation) that excludes "object" and replace OutputSubField.Type with that new
type; refer to OutputSubField, OutputFieldType, and Properties when making the
change so consumers can't declare type: "object" without a schema.
| @@ -0,0 +1,871 @@ | |||
| /* | |||
| Copyright 2024. | |||
There was a problem hiding this comment.
Minor: Copyright year inconsistency.
This file uses Copyright 2024 while other new files in this PR (e.g., llmprovider_types.go, agent_types.go, proposal_types.go) use Copyright 2026. Consider aligning for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/proposal/api/v1alpha1/olsconfig_types.go` at line 2, Update the copyright
header year in the top-of-file comment from 2024 to 2026 to match the other
generated types (e.g., the headers used in llmprovider_types.go, agent_types.go,
proposal_types.go); locate the file's header comment (the package-level/top
comment in olsconfig_types.go) and change the year value to 2026 for
consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/cvo/accept_risks.go (1)
264-273: Harden proposal polling to avoid false positives and fix stale failure text.The current
len(proposals.Items) > 0check can pass on unrelated pre-existing proposals, and the failure message still references conditional update risks.Proposed refinement
- g.By("Checking if the proposal are created") + g.By("Checking if proposals 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, runtimeclient.InNamespace(external.DefaultCVONamespace)) o.Expect(err).NotTo(o.HaveOccurred()) - if len(proposals.Items) == 0 { - return false, nil + for _, p := range proposals.Items { + if p.Name == "test-proposal" { + return true, nil + } } - return true, nil - })).NotTo(o.HaveOccurred(), "no conditional update risk from alert found in ClusterVersion's status") + return false, nil + })).NotTo(o.HaveOccurred(), "no proposals found in ClusterVersion namespace")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/accept_risks.go` around lines 264 - 273, The polling currently treats any existing Proposal as success, causing false positives and leaving an incorrect failure message; modify the wait loop that lists proposalv1alpha1.ProposalList via rtClient.List (namespace external.DefaultCVONamespace) inside wait.PollUntilContextTimeout to filter proposals for the specific alert/caller (e.g., by checking a known label, annotation or ownerReference added when creating the proposal) and only return true when a matching proposal is found, and update the final error text passed to NotTo(o.HaveOccurred()) to reference the correct missing proposal (not "conditional update risk from alert").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cvo/accept_risks.go`:
- Around line 249-250: Rename the g.It test description to the Jira-title format
(e.g. start with [Jira:"Cluster Version Operator"] Create proposals) and add a
short comment immediately above the util.SkipIfNetworkRestricted call explaining
why access to util.FauxinnatiAPIURL is required for this e2e (external endpoint
used to simulate/validate proposal creation), then keep the existing skip by
calling util.SkipIfNetworkRestricted(ctx, c, util.FauxinnatiAPIURL); ensure you
update the g.It text only (the test body and skip call remain) so the test name
and the rationale comment follow repo conventions.
- Around line 53-54: The runtime client is created with
runtimeclient.New(config.GetConfigOrDie(), ...) without registering the
proposalv1alpha1 scheme and ignores the available REST config variable c; update
the setup to register proposalv1alpha1 into the client scheme before calling
runtimeclient.New and pass the existing c (from util.GetRestConfig()) instead of
calling config.GetConfigOrDie(); ensure the scheme registration covers the types
used by ProposalList and other proposal operations. Also add brief E2E-style
comments where the skip is performed to explain the network restriction and the
external endpoint, and update the assertion message used when checking proposals
(the check currently mentions "conditional update risk from alert") to reference
proposals instead of alerts/risks.
---
Nitpick comments:
In `@test/cvo/accept_risks.go`:
- Around line 264-273: The polling currently treats any existing Proposal as
success, causing false positives and leaving an incorrect failure message;
modify the wait loop that lists proposalv1alpha1.ProposalList via rtClient.List
(namespace external.DefaultCVONamespace) inside wait.PollUntilContextTimeout to
filter proposals for the specific alert/caller (e.g., by checking a known label,
annotation or ownerReference added when creating the proposal) and only return
true when a matching proposal is found, and update the final error text passed
to NotTo(o.HaveOccurred()) to reference the correct missing proposal (not
"conditional update risk from alert").
🪄 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: Pro Plus
Run ID: 19fc22b0-5335-4f36-8ac5-7facc9dca7e4
📒 Files selected for processing (2)
pkg/proposal/controller.gotest/cvo/accept_risks.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/proposal/controller.go
openshift-ci
Bot
removed
the
needs-rebase
hongkailiu
force-pushed
the
OTA-1966
branch
8 times, most recently
from
deaae2c to
f583aba
Compare
Member
Author
|
The new case is in the 2nd shard. |
hongkailiu
force-pushed
the
OTA-1966
branch
3 times, most recently
from
5760c92 to
01fdc4c
Compare
Member
Author
|
/retest-required |
1 similar comment
Member
Author
|
/retest-required |
Member
Author
|
Taken from this job: Seems retried automatically. This is new to me. /test e2e-agnostic-ovn-techpreview-serial-2of3 |
hongkailiu
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Member
Author
|
hypershift is broken? /test e2e-hypershift-conformance |
Member
Author
back to normal. run again. /test e2e-agnostic-ovn-techpreview-serial-2of3 |
Member
Author
|
This one is also normal. /test e2e-agnostic-ovn-techpreview-serial-2of3 |
Contributor
|
@hongkailiu: The following tests failed, say
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. |
Draft
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
3 participants
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.
It scaffolds the controller that watches (both available and conditional) updates and creates LightSpeed Proposals.
The OLS types are dumped into
pkg/proposal/api/v1alpha1so that we can create k8s controller-runtime client for the proposals. We will import them when they are available in LS-operator. There is no static client for it. We may switch to it if available in the future to keep the consistent API style of CVO.It creates ONE testing Proposal with reference to a non-existing workflow and a fake request no matter what updates are in the system. We will fix that in other pulls. The clean up of Proposals will be done later too. The current change is to show the logic of controller and the effective API on the Proposal. The new e2e verifies if the Proposal exists.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests