feat: introduce resource status ReconcileForbidden#7422
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:
📝 WalkthroughWalkthroughAdds a new synced/reconciliation reason ChangesReconcileForbidden condition support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/core/v2/condition.go`:
- Around line 333-339: The ReconcileForbidden() constructor currently omits a
user-facing Message; update the returned Condition (in function
ReconcileForbidden) to include a concise, actionable Message string that tells
the user why reconciliation is blocked and what to do next (e.g.,
“Reconciliation is disabled for this resource; enable reconciliation or remove
the forbid flag to allow updates.”). Add this Message field to the Condition
literal so status output includes clear guidance alongside Type, Status, Reason,
and LastTransitionTime.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0dc9f25-caa1-48d7-b78c-f225696f9e5e
📒 Files selected for processing (1)
apis/core/v2/condition.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apis/core/v2/condition.go (1)
66-72: Consider the feature flag requirement for the overall feature.Per coding guidelines, experimental features affecting
apis/**should have feature flag implementations. While this change just defines a new condition reason (which is low risk and additive), I wanted to check: is there a feature gate planned for the overall reconciliation-forbidding behavior when this is wired up in the runtime layer?The guideline is mainly concerned with behavior changes, and since the actual logic will live in crossplane-runtime (per the PR description), the feature flag might be more appropriate there. Just wanted to raise this for visibility in case it hasn't been considered yet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apis/core/v2/condition.go` around lines 66 - 72, The new ConditionReason constant ReasonReconcileForbidden is additive so it doesn't need a feature flag here, but ensure the reconcile-forbidding behavior that will use it is behind a feature gate in the runtime layer (crossplane-runtime); add a short TODO comment next to ReasonReconcileForbidden referencing that runtime must implement a feature flag (and name it, e.g. "EnableReconcileForbid") and ensure the actual logic that enforces forbidden reconciles lives in crossplane-runtime and checks that gate before changing conditions or behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/core/v2/condition.go`:
- Line 342: There is an extra closing brace that causes a syntax error after the
ReconcileForbidden() function; remove the stray '}' that follows the end of the
ReconcileForbidden() function so the braces properly balance and the file
compiles.
---
Nitpick comments:
In `@apis/core/v2/condition.go`:
- Around line 66-72: The new ConditionReason constant ReasonReconcileForbidden
is additive so it doesn't need a feature flag here, but ensure the
reconcile-forbidding behavior that will use it is behind a feature gate in the
runtime layer (crossplane-runtime); add a short TODO comment next to
ReasonReconcileForbidden referencing that runtime must implement a feature flag
(and name it, e.g. "EnableReconcileForbid") and ensure the actual logic that
enforces forbidden reconciles lives in crossplane-runtime and checks that gate
before changing conditions or behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3acfc08-8ea6-44c3-bebe-ba8f86706b66
📒 Files selected for processing (1)
apis/core/v2/condition.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/core/v2/condition.go`:
- Around line 342-350: There are two identical declarations of the function
ReconcileForbidden(), causing a redeclaration build error; remove the duplicate
definition so only one ReconcileForbidden() function remains, and if the second
was intended as the replacement, keep that implementation and delete the other;
search for the symbol ReconcileForbidden to verify no other variants exist and
run go build to confirm the redeclaration is resolved.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f727894-f7f7-40ad-ae86-5a42250c4c6d
📒 Files selected for processing (1)
apis/core/v2/condition.go
bobh66
left a comment
There was a problem hiding this comment.
Please check the duplicate function definition, otherwise LGTM. Thanks!
Signed-off-by: Julian Wefers <julian@wefers.page>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: jwefers <851823+jwefers@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: jwefers <851823+jwefers@users.noreply.github.com>
Signed-off-by: Julian Wefers <851823+jwefers@users.noreply.github.com>
54d6b3e to
7ad60c1
Compare
|
done. thanks as well for having a look. the corresponding PR for crossplane-runtime is now also in good shape (imho) |
This is a precondition for crossplane/crossplane-runtime#926 due to upstream changes.