feat(webhook): unblock cluster deletion by removing deletion schedule handling#2082
Open
jellonek wants to merge 3 commits into
Open
feat(webhook): unblock cluster deletion by removing deletion schedule handling#2082jellonek wants to merge 3 commits into
jellonek wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the “cluster deletion scheduling via annotations” mechanism (mutating/validating webhook logic, controller handling, and related helpers) so Cluster objects can be deleted directly, and updates docs/e2e accordingly.
Changes:
- Removed deletion-scheduling annotations and reasons (
greenhouse.sap/delete-cluster,greenhouse.sap/deletion-schedule,ScheduledDeletionReason) and all associated logic across webhook/controller/util layers. - Updated e2e and user documentation to off-board clusters via direct deletion instead of scheduling.
- Cleaned up tests/helpers that previously set or validated deletion scheduling.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/lifecycle/reconcile.go | Removes ScheduledDeletionReason constant from lifecycle reasons. |
| internal/webhook/v1alpha1/cluster_webhook.go | Removes defaulting/validation/delete logic tied to deletion scheduling annotations. |
| internal/webhook/v1alpha1/cluster_webhook_test.go | Removes deletion-scheduling expectations/cases (but currently leaves failing/invalid test cases; see comments). |
| internal/test/util.go | Removes helper that annotated clusters for scheduled deletion; deletes clusters directly in tests. |
| internal/test/assert.go | Stops special-casing Cluster deletion preparation via annotations in generic deletion helper. |
| internal/controller/plugin/util.go | Removes requeue/condition logic that depended on cluster deletion scheduling. |
| internal/controller/plugin/pluginpreset_controller.go | Removes filtering/reconciliation tied to scheduled deletion (but currently breaks PluginPreset reconciliation; see comments). |
| internal/controller/plugin/plugin_controller.go | Removes requeue logic that blocked plugin reconciliation during scheduled deletion. |
| internal/controller/cluster/status_test.go | Removes test asserting delete condition with ScheduledDeletionReason. |
| internal/controller/cluster/cluster_status.go | Removes Delete condition handling based on deletion schedule annotations. |
| internal/controller/cluster/cluster_controller.go | Removes controller-driven deletion based on schedule elapsed time. |
| internal/clientutil/util.go | Deletes schedule-extraction / deletion-filtering utility functions. |
| e2e/shared/cluster.go | Off-boarding flow now directly deletes the Cluster resource. |
| e2e/cluster/expect/expect.go | Removes expectation helper for “deletion is scheduled in ~48h”. |
| e2e/cluster/e2e_test.go | Removes e2e test that asserted deletion scheduling behavior. |
| docs/user-guides/cluster/offboarding.md | Rewrites off-boarding docs to direct deletion (has Markdown issues; see comments). |
| api/well_known.go | Removes well-known constants for deletion scheduling annotations. |
Comments suppressed due to low confidence (2)
internal/webhook/v1alpha1/cluster_webhook_test.go:130
- This test case still expects cluster creation to be denied due to a deletion-marker annotation, but the webhook logic that enforced this has been removed. The test should be updated to reflect the new behavior (e.g., allow creation).
Entry("it should deny creation of cluster with deletion marker annotation",
test.NewCluster(test.Ctx, "test-cluster", test.TestNamespace,
test.WithClusterLabel(greenhouseapis.LabelKeyOwnedBy, teamWithSupportGroupName),
),
true,
),
internal/webhook/v1alpha1/cluster_webhook_test.go:207
- ValidateUpdateCluster no longer validates deletion scheduling, but these entries still assert behavior for "valid/invalid deletion schedule" (including expecting an error). This will make the suite fail and should be updated/removed.
Entry("it should allow update with valid deletion schedule",
test.NewCluster(test.Ctx, "test-cluster", test.TestNamespace,
test.WithClusterLabel(greenhouseapis.LabelKeyOwnedBy, teamWithSupportGroupName),
),
false,
),
Entry("it should deny update with invalid deletion schedule",
test.NewCluster(test.Ctx, "test-cluster", test.TestNamespace,
test.WithClusterLabel(greenhouseapis.LabelKeyOwnedBy, teamWithSupportGroupName),
),
true,
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c59f31b to
3ddb663
Compare
… handling Closes #1947 Signed-off-by: Piotr Skamruk <piotr.skamruk@gmail.com>
Signed-off-by: Piotr Skamruk <piotr.skamruk@gmail.com>
Signed-off-by: Piotr Skamruk <piotr.skamruk@gmail.com>
3ddb663 to
5a44901
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
This PR unblocks deletion of cluster object by removing the logic around cluster deletion scheduling through annotations.
All other required parts of #1947 (deletion mechanism, tests, handling plugins cleanup) were there already in place.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Added to documentation?
Checklist