Open
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces legacy cluster/readiness conditions with Flux HelmRelease–centric conditions (HelmReleaseCreatedCondition, HelmReleaseDeployedCondition, ExposedServicesSyncedCondition), adds fetchReleaseStatus, updates Flux reconcile/error paths and client-getter flows, refreshes tests/e2e, and bumps Flux CLI dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant FluxHelmRelease as Flux HelmRelease
participant Cluster as Kubernetes Cluster
participant Store as PluginStatus
User->>Controller: create/update Plugin
Controller->>Store: set HelmReleaseCreatedCondition = Unknown
Controller->>Cluster: init client / verify cluster access
alt cluster access success
Controller->>Store: set HelmReleaseCreatedCondition = True
Controller->>FluxHelmRelease: create/update HelmRelease (flux)
FluxHelmRelease-->>Controller: create/update result (ok | config invalid | stalled)
alt created / ok
Controller->>FluxHelmRelease: fetch HelmRelease status
FluxHelmRelease-->>Controller: Deployed + Exposed services info
Controller->>Store: set HelmReleaseDeployedCondition = True
Controller->>Store: set ExposedServicesSyncedCondition = True/False
else config invalid / stalled
Controller->>Store: set HelmReleaseCreatedCondition = False (FluxHelmReleaseConfigInvalid)
Controller->>Store: set HelmReleaseDeployedCondition = False (FluxHelmReleaseStalled)
end
else cluster access failure
Controller->>Store: set HelmReleaseCreatedCondition = False (ClusterAccessFailed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
24-25:⚠️ Potential issue | 🟠 MajorAlign Kubernetes module versions between
replaceandrequiredirectives.Lines 24–25 pin
k8s.io/component-baseandk8s.io/kubectltov0.35.0in replace directives, while lines 215 and 217 require them atv0.35.1. Sincereplaceoverridesrequirein Go modules, thev0.35.1versions are effectively ignored, creating dependency skew.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 24 - 25, The replace directives for k8s.io/component-base and k8s.io/kubectl are pinned to v0.35.0 while the require directives list v0.35.1, causing a version skew; update the replace entries for the module names k8s.io/component-base and k8s.io/kubectl to match the required v0.35.1 (or alternatively change the require lines to v0.35.0) so the replace and require versions are consistent and no longer override the intended versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/plugin/plugin_controller_flux.go`:
- Around line 336-339: When r.Get(ctx, NamespacedName{Name: plugin.Name,
Namespace: plugin.Namespace}, helmRelease) fails, the code currently only sets
ExposedServicesSyncedCondition; also clear HelmReleaseDeployedCondition to avoid
stale True state. Update the error branch that calls
plugin.SetCondition(greenhousemetav1alpha1.FalseCondition(...,
greenhousev1alpha1.ExposedServicesSyncedCondition, ...)) to additionally call
plugin.SetCondition(greenhousemetav1alpha1.FalseCondition(...,
greenhousev1alpha1.HelmReleaseDeployedCondition, "failed to load Flux
HelmRelease: "+err.Error())) so both conditions reflect the fetch failure.
- Around line 269-280: The early-return for pluginDefinitionSpec.HelmChart ==
nil currently sets Ready=True but leaves deployment-related conditions unset;
update this branch to explicitly set HelmReleaseDeployedCondition and
ExposedServicesSyncedCondition on plugin.Status.Conditions (and/or the
readyCondition siblings) to deterministic values (e.g., metav1.ConditionFalse
with Reason "NotApplicable" and a message like "plugin not backed by a Helm
chart") and ensure plugin.Status.HelmReleaseStatus and
plugin.Status.ExposedServices remain nil and plugin.Status.Version is set as
before so the UI won’t show Unknown for those conditions; locate the branch
checking pluginDefinitionSpec.HelmChart, modify the code that sets plugin.Status
and readyCondition to also set the two named conditions
(HelmReleaseDeployedCondition, ExposedServicesSyncedCondition) with the chosen
status, reason, and message.
In `@internal/controller/plugin/util.go`:
- Around line 103-106: The condition is being set to True when cluster retrieval
failed; change the call to use greenhousemetav1alpha1.FalseCondition instead of
TrueCondition so that
plugin.SetCondition(greenhousemetav1alpha1.FalseCondition(...)) is used for
greenhousev1alpha1.HelmReleaseCreatedCondition with
greenhousev1alpha1.ClusterAccessFailedReason and the error message (err.Error())
to correctly reflect the failure state.
- Around line 86-136: The initClientGetter function currently takes plugin by
value so plugin.SetCondition(...) mutates only the local copy; change the
function signature to accept a pointer (e.g., plugin *greenhousev1alpha1.Plugin)
and update all internal uses accordingly so SetCondition updates the real
object, then update the call sites (notably in plugin_controller_flux.go) to
pass the plugin pointer instead of a value; ensure any nil checks or method
receivers still compile after this pointer change.
---
Outside diff comments:
In `@go.mod`:
- Around line 24-25: The replace directives for k8s.io/component-base and
k8s.io/kubectl are pinned to v0.35.0 while the require directives list v0.35.1,
causing a version skew; update the replace entries for the module names
k8s.io/component-base and k8s.io/kubectl to match the required v0.35.1 (or
alternatively change the require lines to v0.35.0) so the replace and require
versions are consistent and no longer override the intended versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47e921ab-d74e-44e9-af40-c3c86715619b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
api/v1alpha1/plugin_types.goe2e/plugin/scenarios/flux_controller.gogo.modinternal/controller/plugin/plugin_controller_flux.gointernal/controller/plugin/plugin_controller_flux_test.gointernal/controller/plugin/util.go
IvoGoman
force-pushed
the
refactor/plugin-conditions
branch
from
142f177 to
e0159da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/plugin/scenarios/flux_controller.go (1)
247-259:⚠️ Potential issue | 🔴 CriticalController does not set
HelmReleaseDeployedConditionfor UI-only plugins, causing test failure.The test expects
HelmReleaseDeployedConditionto beTruefor UI-only plugins (lines 248-249), but the controller never sets this condition. InEnsureFluxCreated(lines 77-84 ofplugin_controller_flux.go), onlyHelmReleaseCreatedConditionis set toUnknownfor non-Helm-backed plugins;HelmReleaseDeployedConditionis not touched. Additionally,computeReadyConditionFluxreturns early for UI-only plugins without executing the code that would setHelmReleaseDeployedCondition. The condition will remain unset ornil, notTrue, causing the test assertion to fail.The controller must explicitly set
HelmReleaseDeployedConditiontoTruefor UI-only plugins, likely inEnsureFluxCreatedalongside the existingHelmReleaseCreatedConditionupdate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/plugin/scenarios/flux_controller.go` around lines 247 - 259, The controller currently never sets HelmReleaseDeployedCondition for UI-only plugins; update EnsureFluxCreated in plugin_controller_flux.go to explicitly set HelmReleaseDeployedCondition to metav1.ConditionTrue (similar to how HelmReleaseCreatedCondition is set to Unknown) for non-Helm-backed/UI-only plugins, and ensure computeReadyConditionFlux does not return before applying that condition (or move the HelmReleaseDeployedCondition update into computeReadyConditionFlux but executed for UI-only plugins) so the condition is present and True in plugin.Status.
♻️ Duplicate comments (1)
internal/controller/plugin/plugin_controller_flux.go (1)
269-280:⚠️ Potential issue | 🟠 MajorUI-only path leaves
HelmReleaseDeployedConditionandExposedServicesSyncedConditionunset.When
pluginDefinitionSpec.HelmChart == nil, this early-return path setsReadyCondition=Truebut does not updateHelmReleaseDeployedConditionorExposedServicesSyncedCondition. These conditions will remainUnknownfrom initialization, which:
- Creates inconsistent condition states between Helm-backed and UI-only plugins
- Will cause the e2e test at
e2e/plugin/scenarios/flux_controller.go:247-249to fail (it expectsHelmReleaseDeployedCondition=True)This was flagged in a previous review and appears unaddressed.
🔧 Proposed fix
if pluginDefinitionSpec.HelmChart == nil { plugin.Status.HelmChart = nil plugin.Status.HelmReleaseStatus = nil plugin.Status.Version = pluginDefinitionSpec.Version plugin.Status.ExposedServices = nil + plugin.SetCondition(greenhousemetav1alpha1.TrueCondition( + greenhousev1alpha1.HelmReleaseDeployedCondition, + greenhousev1alpha1.PluginDefinitionNotBackedByHelmChartReason, + "No HelmRelease required", + )) + plugin.SetCondition(greenhousemetav1alpha1.TrueCondition( + greenhousev1alpha1.ExposedServicesSyncedCondition, + greenhousev1alpha1.PluginDefinitionNotBackedByHelmChartReason, + "No exposed services to sync", + )) readyCondition.Status = metav1.ConditionTrue readyCondition.Message = "ready" readyCondition.Reason = "" return readyCondition }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/plugin/plugin_controller_flux.go` around lines 269 - 280, The early-return when pluginDefinitionSpec.HelmChart == nil sets ReadyCondition but leaves HelmReleaseDeployedCondition and ExposedServicesSyncedCondition unset (Unknown); update the early-return branch to explicitly set HelmReleaseDeployedCondition = metav1.ConditionTrue and ExposedServicesSyncedCondition = metav1.ConditionTrue on the plugin status (alongside setting plugin.Status.HelmChart/HelmReleaseStatus/Version/ExposedServices as already done) and populate their Message/Reason appropriately (e.g., "ui-only plugin" / empty reason) so UI-only plugins report consistent True conditions and satisfy the e2e expectation; make these changes where readyCondition is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/plugin/util.go`:
- Around line 91-95: The recovery logic for HelmReleaseCreatedCondition is
inconsistent between the in-cluster and remote-cluster paths: change the
remote-cluster branch that currently only sets
plugin.SetCondition(greenhousemetav1alpha1.TrueCondition(...)) when c != nil &&
c.Reason == greenhousev1alpha1.ClusterAccessFailedReason to the same predicate
used in the in-cluster path (set True when c == nil || c.Reason ==
greenhousev1alpha1.ClusterAccessFailedReason). Update the logic around
plugin.Status.GetConditionByType(greenhousev1alpha1.HelmReleaseCreatedCondition)
and the plugin.SetCondition(...) call in the remote path (also apply the same
fix to the similar block at the other occurrence) so newly-created plugins get
HelmReleaseCreatedCondition=True consistently.
---
Outside diff comments:
In `@e2e/plugin/scenarios/flux_controller.go`:
- Around line 247-259: The controller currently never sets
HelmReleaseDeployedCondition for UI-only plugins; update EnsureFluxCreated in
plugin_controller_flux.go to explicitly set HelmReleaseDeployedCondition to
metav1.ConditionTrue (similar to how HelmReleaseCreatedCondition is set to
Unknown) for non-Helm-backed/UI-only plugins, and ensure
computeReadyConditionFlux does not return before applying that condition (or
move the HelmReleaseDeployedCondition update into computeReadyConditionFlux but
executed for UI-only plugins) so the condition is present and True in
plugin.Status.
---
Duplicate comments:
In `@internal/controller/plugin/plugin_controller_flux.go`:
- Around line 269-280: The early-return when pluginDefinitionSpec.HelmChart ==
nil sets ReadyCondition but leaves HelmReleaseDeployedCondition and
ExposedServicesSyncedCondition unset (Unknown); update the early-return branch
to explicitly set HelmReleaseDeployedCondition = metav1.ConditionTrue and
ExposedServicesSyncedCondition = metav1.ConditionTrue on the plugin status
(alongside setting
plugin.Status.HelmChart/HelmReleaseStatus/Version/ExposedServices as already
done) and populate their Message/Reason appropriately (e.g., "ui-only plugin" /
empty reason) so UI-only plugins report consistent True conditions and satisfy
the e2e expectation; make these changes where readyCondition is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02c33feb-a507-43a3-b9b9-65b8755eb025
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
api/v1alpha1/plugin_types.goe2e/plugin/scenarios/flux_controller.gogo.modinternal/controller/plugin/plugin_controller_flux.gointernal/controller/plugin/plugin_controller_flux_test.gointernal/controller/plugin/util.go
This was
linked to
issues
Mar 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 215-217: The replace block is pinning k8s modules back to v0.35.0
and negating the require upgrades; update the replace entries that reference
k8s.io/component-base and k8s.io/kubectl (and any k8s.io/kube-openapi entry if
present) so they point to v0.35.1 to match the require section, or remove those
specific replace lines entirely if they are no longer needed; ensure the module
paths in the replace block exactly match the ones in require
(k8s.io/component-base, k8s.io/kubectl, k8s.io/kube-openapi) so the bumped
versions take effect.
9453651 to
f7a3204
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
go.mod (1)
215-217:⚠️ Potential issue | 🟠 Major
replacestill overrides the k8s bumps back tov0.35.0.
k8s.io/component-baseandk8s.io/kubectlare upgraded inrequire, but Line 24 and Line 25 still pin them tov0.35.0viareplace, so the upgrade does not take effect.🔧 Proposed fix
- k8s.io/component-base => k8s.io/component-base v0.35.0 - k8s.io/kubectl => k8s.io/kubectl v0.35.0 + k8s.io/component-base => k8s.io/component-base v0.35.1 + k8s.io/kubectl => k8s.io/kubectl v0.35.1#!/bin/bash # Verify require vs replace versions for affected k8s modules. python - <<'PY' import re from pathlib import Path text = Path("go.mod").read_text() mods = ["k8s.io/component-base", "k8s.io/kubectl"] for mod in mods: req = re.search(rf'^\s*{re.escape(mod)}\s+([^\s]+)\s+//\s+indirect\s*$', text, re.M) rep = re.search(rf'^\s*{re.escape(mod)}\s*=>\s*{re.escape(mod)}\s+([^\s]+)\s*$', text, re.M) req_v = req.group(1) if req else None rep_v = rep.group(1) if rep else None print(f"{mod}: require={req_v} replace={rep_v}") if req_v and rep_v and req_v != rep_v: print("MISMATCH") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 215 - 217, The go.mod contains replace entries that pin k8s modules back to v0.35.0, overriding the upgraded require lines; update or remove the replace directives for k8s.io/component-base and k8s.io/kubectl so their versions match the require entries (e.g., v0.35.1) or delete the replace lines entirely if they are no longer needed; look for the replace statements referencing "k8s.io/component-base => k8s.io/component-base" and "k8s.io/kubectl => k8s.io/kubectl" and change the version token to the same version used in the require block or remove those replace entries to allow the require upgrades to take effect.internal/controller/plugin/plugin_controller_flux.go (1)
330-337:⚠️ Potential issue | 🟠 MajorUI-only path leaves deployment/sync conditions stale (
Unknown).Line 331 returns early without setting
HelmReleaseDeployedConditionandExposedServicesSyncedCondition. These stay in initializedUnknownstate, which creates inconsistent status semantics for non-Helm plugins.🔧 Proposed fix
if pluginDefinitionSpec.HelmChart == nil { pluginStatus.HelmChart = nil pluginStatus.HelmReleaseStatus = nil pluginStatus.Version = pluginDefinitionSpec.Version pluginStatus.ExposedServices = nil + plugin.SetCondition(greenhousemetav1alpha1.TrueCondition( + greenhousev1alpha1.HelmReleaseDeployedCondition, + greenhousev1alpha1.PluginDefinitionNotBackedByHelmChartReason, + "No HelmRelease required", + )) + plugin.SetCondition(greenhousemetav1alpha1.TrueCondition( + greenhousev1alpha1.ExposedServicesSyncedCondition, + greenhousev1alpha1.PluginDefinitionNotBackedByHelmChartReason, + "No exposed services to sync", + )) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/plugin/plugin_controller_flux.go` around lines 330 - 337, The early return for non-Helm plugins leaves HelmReleaseDeployedCondition and ExposedServicesSyncedCondition in Unknown; update the UI-only branch (where pluginDefinitionSpec.HelmChart == nil and pluginStatus.HelmChart/HelmReleaseStatus/ExposedServices are set) to also set explicit status conditions for HelmReleaseDeployedCondition and ExposedServicesSyncedCondition (e.g., mark them False/NotApplicable with a clear reason/message like "UIOnly" or "NoHelmChart") so the status is consistent for non-Helm plugins; modify the code that manipulates pluginStatus conditions (the same block that touches pluginStatus.HelmChart, pluginStatus.HelmReleaseStatus, pluginStatus.Version, pluginStatus.ExposedServices) to set those two conditions accordingly.
🧹 Nitpick comments (1)
e2e/plugin/scenarios/flux_controller.go (1)
122-124: Assert the full new condition contract, not justHelmReleaseDeployed.These checks only validate
HelmReleaseDeployedCondition. Consider also assertingHelmReleaseCreatedConditionandExposedServicesSyncedConditionhere to protect the full refactor intent from regressions.Also applies to: 247-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/plugin/scenarios/flux_controller.go` around lines 122 - 124, Currently the test only asserts HelmReleaseDeployedCondition; update the assertions to validate the full condition contract by also retrieving and asserting HelmReleaseCreatedCondition and ExposedServicesSyncedCondition from plugin.Status.StatusConditions (use plugin.Status.StatusConditions.GetConditionByType for each) and check that each condition is non-nil and has Status metav1.ConditionTrue; apply the same additional assertions in the other identical check site (the second occurrence near the end of the file) so all three conditions are validated wherever HelmReleaseDeployedCondition is currently asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/plugin/plugin_controller_flux.go`:
- Around line 446-447: The Helm-backed completion path computes pluginVersion
but never updates pluginStatus.Version, leaving .status.version stale; after
computing pluginVersion (in the same block where HelmReleaseStatus and
ExposedServices are set) assign pluginStatus.Version = pluginVersion so the new
version is persisted (i.e., set pluginStatus.Version before the status
save/update that follows the HelmReleaseStatus and ExposedServices assignment).
---
Duplicate comments:
In `@go.mod`:
- Around line 215-217: The go.mod contains replace entries that pin k8s modules
back to v0.35.0, overriding the upgraded require lines; update or remove the
replace directives for k8s.io/component-base and k8s.io/kubectl so their
versions match the require entries (e.g., v0.35.1) or delete the replace lines
entirely if they are no longer needed; look for the replace statements
referencing "k8s.io/component-base => k8s.io/component-base" and "k8s.io/kubectl
=> k8s.io/kubectl" and change the version token to the same version used in the
require block or remove those replace entries to allow the require upgrades to
take effect.
In `@internal/controller/plugin/plugin_controller_flux.go`:
- Around line 330-337: The early return for non-Helm plugins leaves
HelmReleaseDeployedCondition and ExposedServicesSyncedCondition in Unknown;
update the UI-only branch (where pluginDefinitionSpec.HelmChart == nil and
pluginStatus.HelmChart/HelmReleaseStatus/ExposedServices are set) to also set
explicit status conditions for HelmReleaseDeployedCondition and
ExposedServicesSyncedCondition (e.g., mark them False/NotApplicable with a clear
reason/message like "UIOnly" or "NoHelmChart") so the status is consistent for
non-Helm plugins; modify the code that manipulates pluginStatus conditions (the
same block that touches pluginStatus.HelmChart, pluginStatus.HelmReleaseStatus,
pluginStatus.Version, pluginStatus.ExposedServices) to set those two conditions
accordingly.
---
Nitpick comments:
In `@e2e/plugin/scenarios/flux_controller.go`:
- Around line 122-124: Currently the test only asserts
HelmReleaseDeployedCondition; update the assertions to validate the full
condition contract by also retrieving and asserting HelmReleaseCreatedCondition
and ExposedServicesSyncedCondition from plugin.Status.StatusConditions (use
plugin.Status.StatusConditions.GetConditionByType for each) and check that each
condition is non-nil and has Status metav1.ConditionTrue; apply the same
additional assertions in the other identical check site (the second occurrence
near the end of the file) so all three conditions are validated wherever
HelmReleaseDeployedCondition is currently asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4eb4b16-8596-458b-b8c6-57b79e00c489
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
api/v1alpha1/plugin_types.goe2e/plugin/scenarios/flux_controller.gogo.modinternal/controller/plugin/plugin_controller_flux.gointernal/controller/plugin/plugin_controller_flux_test.gointernal/controller/plugin/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/plugin/plugin_controller_flux_test.go
IvoGoman
force-pushed
the
refactor/plugin-conditions
branch
from
f7a3204 to
6491466
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/plugin/plugin_controller_flux.go (1)
444-448: Version update logic is correct but could preserve previous value on failure.The
pluginStatus.Versionis updated (addressing past review), but unlikehelmChartReferencewhich preserves the previous value on failure (lines 439-443),Versiongets reset to empty string when deployment fails. Consider preserving the previous version to maintain consistency.Suggested improvement (optional)
+ // Preserve previous version if current deployment failed + if pluginVersion == "" && plugin.Status.Version != "" { + pluginVersion = plugin.Status.Version + } + pluginStatus.HelmChart = helmChartReference pluginStatus.Version = pluginVersion pluginStatus.HelmReleaseStatus = releaseStatus🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/plugin/plugin_controller_flux.go` around lines 444 - 448, The pluginStatus.Version is being overwritten with pluginVersion even when deployment fails, losing the previous value; change the assignment so it preserves the existing pluginStatus.Version on failure by only setting pluginStatus.Version to pluginVersion when pluginVersion is non-empty or when deployment succeeded (or otherwise fall back to the existing pluginStatus.Version). Mirror the existing pattern used for helmChartReference: compute/validate the new version first and assign to pluginStatus.Version only when valid, otherwise retain the prior value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/plugin/plugin_controller_flux.go`:
- Around line 444-448: The pluginStatus.Version is being overwritten with
pluginVersion even when deployment fails, losing the previous value; change the
assignment so it preserves the existing pluginStatus.Version on failure by only
setting pluginStatus.Version to pluginVersion when pluginVersion is non-empty or
when deployment succeeded (or otherwise fall back to the existing
pluginStatus.Version). Mirror the existing pattern used for helmChartReference:
compute/validate the new version first and assign to pluginStatus.Version only
when valid, otherwise retain the prior value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31005b18-b2bf-49a1-8835-f59d30a24bf8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
api/v1alpha1/plugin_types.goe2e/plugin/scenarios/flux_controller.gogo.modinternal/controller/plugin/plugin_controller_flux.gointernal/controller/plugin/plugin_controller_flux_test.gointernal/controller/plugin/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/plugin/scenarios/flux_controller.go
- internal/controller/plugin/plugin_controller_flux_test.go
There was a problem hiding this comment.
Pull request overview
This PR restructures the Plugin status conditions by replacing multiple legacy conditions (StatusUpToDate, WorkloadReady, HelmChartTestSucceeded, ClusterAccessReady, HelmReconcileFailed, HelmDriftDetected, WaitingForDependencies, RetriesExhausted) with three clearer ones: HelmReleaseCreated, HelmReleaseDeployed, and ExposedServicesSynced. It also adds a new fluxcd/cli-utils dependency for the ConditionStalled constant, renames reconcilePluginStatus to fetchReleaseStatus, and changes initClientGetter to take a pointer receiver. Deprecated conditions are cleaned up during plugin initialization.
Changes:
- Replaces 8 legacy Plugin conditions with 3 new semantically clear conditions (
HelmReleaseCreated,HelmReleaseDeployed,ExposedServicesSynced) and updates all condition-setting logic accordingly. - Adds
fetchReleaseStatuswith an early return for UI-only plugins (no HelmChart), and introduces stalled release detection via FluxConditionStalled. - Updates E2E and unit tests to reflect the new condition types, and adds a
deprecatedConditionslist for cleaning up old conditions on existing plugins.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/plugin_types.go | Defines new condition types (HelmReleaseCreated, HelmReleaseDeployed, ExposedServicesSynced) and reason constants, removes old ones |
| internal/controller/plugin/util.go | Updates exposedConditions, adds deprecatedConditions cleanup in InitPluginStatus, changes initClientGetter to pointer receiver and uses new conditions |
| internal/controller/plugin/plugin_controller_flux.go | Major refactor: new condition semantics, fetchReleaseStatus with early return for UI-only, stalled detection, ready condition computation changes |
| internal/controller/plugin/plugin_controller_flux_test.go | Updates unit test expectations to match new condition types |
| e2e/plugin/scenarios/flux_controller.go | Updates E2E test expectations to match new condition types |
| go.mod | Adds fluxcd/cli-utils dependency, updates several indirect deps |
| go.sum | Updated checksums for new/updated dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+247
to
257
| helmReleaseDeployed := plugin.Status.StatusConditions.GetConditionByType(greenhousev1alpha1.HelmReleaseDeployedCondition) | ||
| g.Expect(helmReleaseDeployed).ToNot(BeNil(), "Plugin HelmReleaseDeployed condition must be set") | ||
| g.Expect(helmReleaseDeployed.Status).To(Equal(metav1.ConditionTrue), "Plugin HelmReleaseDeployed condition must be true") | ||
|
|
||
| ready := plugin.Status.StatusConditions.GetConditionByType(greenhousemetav1alpha1.ReadyCondition) | ||
| g.Expect(ready).ToNot(BeNil(), "Plugin Ready condition must be set") | ||
| g.Expect(ready.Status).To(Equal(metav1.ConditionTrue), "Plugin Ready condition must be true") | ||
|
|
||
| statusUpToDate := plugin.Status.StatusConditions.GetConditionByType(greenhousev1alpha1.StatusUpToDateCondition) | ||
| g.Expect(statusUpToDate).ToNot(BeNil(), "Plugin StatusUpToDate condition must be set") | ||
| g.Expect(statusUpToDate.Status).To(Equal(metav1.ConditionFalse), "Plugin statusUpToDate condition must be false") | ||
|
|
||
| g.Expect(plugin.Status.ExposedServices).To(BeEmpty(), "exposed services in plugin status should be empty") | ||
| g.Expect(plugin.Status.UIApplication).To(Equal(testPluginDefinition.Spec.UIApplication), "UIApplication in plugin status should be set correctly") | ||
| g.Expect(plugin.Status.HelmReleaseStatus.Status).To(Equal("unknown"), "HelmReleaseStatus in plugin status should be unknown") |
There was a problem hiding this comment.
For UI-only plugins (where HelmChart == nil), the HelmReleaseDeployedCondition is initialized to Unknown by InitPluginStatus and is never set to True anywhere in the UI-only code path. However, this E2E test expects it to be ConditionTrue.
Additionally, fetchReleaseStatus sets pluginStatus.HelmReleaseStatus = nil for UI-only plugins (line 333 of plugin_controller_flux.go), so line 257 (plugin.Status.HelmReleaseStatus.Status) will cause a nil pointer dereference panic.
The test expectations need to be updated to match the actual behavior, or the controller code needs to explicitly handle the UI-only case by setting HelmReleaseDeployedCondition appropriately and not nil-ing out HelmReleaseStatus.
Comment on lines
+330
to
+336
| // early return if the plugin is not backed by a Helm chart, to avoid unnecessary attempts to fetch the Helm release and exposed services | ||
| if pluginDefinitionSpec.HelmChart == nil { | ||
| pluginStatus.HelmChart = nil | ||
| pluginStatus.HelmReleaseStatus = nil | ||
| pluginStatus.Version = pluginDefinitionSpec.Version | ||
| pluginStatus.ExposedServices = nil | ||
| return |
There was a problem hiding this comment.
Setting pluginStatus.HelmReleaseStatus = nil for UI-only plugins (no HelmChart) will break the E2E test at e2e/plugin/scenarios/flux_controller.go:257, which accesses plugin.Status.HelmReleaseStatus.Status and expects "unknown". This will cause a nil pointer dereference.
Previously (reconcilePluginStatus), there was no early return for nil HelmChart, so HelmReleaseStatus was always set to the initialized {Status: "unknown"}. Either keep the old behavior by not nil-ing out HelmReleaseStatus (or set it to &HelmReleaseStatus{Status: "unknown"}), or update the E2E test to handle nil HelmReleaseStatus for UI-only plugins.
removes StatusUpToDate, WorkloadReady, HelmChartTestSucceeded, ClusterAccessReady, HelmReconcileFailed, HelmDriftDetected, WaitingForDependencies, RetriesExhausted adds HelmReleaseCreatedCondition to show if the Plugin spec can be used to create the Flux HelmRelease adds HelmReleaseDeployed to show that the Flux HelmRelease was deployed and the status is synced adds ExposedServicesSynced to show that the list in the status was synced
IvoGoman
force-pushed
the
refactor/plugin-conditions
branch
from
6491466 to
d7fb981
Compare
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
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.
Description
removes StatusUpToDate, WorkloadReady, HelmChartTestSucceeded, ClusterAccessReady, HelmReconcileFailed, HelmDriftDetected, WaitingForDependencies, RetriesExhausted
adds HelmReleaseCreatedCondition to show if the Plugin spec can be used to create the Flux HelmRelease
adds HelmReleaseDeployed to show that the Flux HelmRelease was deployed and the status is synced
adds ExposedServicesSynced to show that the list in the status was synced
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests