Skip to content

feat: delete clusters when shoot is deleted (#45)#69

Open
jellonek wants to merge 1 commit into
mainfrom
shoot-removal
Open

feat: delete clusters when shoot is deleted (#45)#69
jellonek wants to merge 1 commit into
mainfrom
shoot-removal

Conversation

@jellonek

@jellonek jellonek commented Jul 6, 2026

Copy link
Copy Markdown

Closes #45

Copilot AI review requested due to automatic review settings July 6, 2026 13:03
@jellonek jellonek requested a review from a team as a code owner July 6, 2026 13:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements cleanup of Greenhouse Cluster resources when the corresponding Gardener Shoot is deleted (404 on previously existing Shoot), and documents the new behavior/events.

Changes:

  • Add logic in the Shoot controller to delete an existing Greenhouse Cluster when the Shoot can no longer be fetched due to NotFound.
  • Emit a new ClusterDeleted event and document it in the README.
  • Add a test intended to validate cluster deletion behavior on Shoot removal.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
README.md Updates documentation to reflect cluster cleanup behavior and adds the ClusterDeleted event.
controller/shoot/shoot_controller.go Implements deletion of the Greenhouse Cluster when the Shoot is deleted and emits a ClusterDeleted event.
controller/shoot/shoot_controller_test.go Adds a new test case intended to cover Shoot removal leading to Cluster cleanup.
Comments suppressed due to low confidence (1)

controller/shoot/shoot_controller.go:120

  • existingClusterFound is computed incorrectly and will be false both when the Cluster exists (err == nil) and when it is NotFound, but true for non-NotFound errors. This prevents cluster deletion on Shoot deletion and can also cause an attempted delete of an empty Cluster object when the initial Get fails for transient reasons.

Handle the Cluster Get result explicitly: set existingClusterFound = true only when err == nil, return early on non-NotFound errors, and keep it false on NotFound.

	existingClusterFound := false
	err := r.GreenhouseClient.Get(ctx, client.ObjectKey{Name: req.Name, Namespace: r.CareInstruction.Namespace}, &existingCluster)
	if err == nil {
		// Cluster exists - check ownership
		if ownerLabel, hasLabel := existingCluster.Labels[v1alpha1.CareInstructionLabel]; hasLabel && ownerLabel != r.CareInstruction.Name {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread controller/shoot/shoot_controller.go
Comment thread controller/shoot/shoot_controller_test.go
Comment thread README.md Outdated
Comment thread README.md Outdated
@jellonek jellonek force-pushed the shoot-removal branch 3 times, most recently from ab95f25 to 612585f Compare July 10, 2026 06:51
@jellonek jellonek requested a review from Copilot July 10, 2026 06:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comment thread controller/shoot/shoot_controller.go
Comment thread controller/shoot/shoot_controller_test.go Outdated
Comment thread controller/shoot/shoot_controller_test.go
Comment on lines +1039 to +1040
It("should handle shoot removal", func() {
shoot := &gardenerv1beta1.Shoot{
@jellonek jellonek force-pushed the shoot-removal branch 2 times, most recently from fcd742c to 5ba47dc Compare July 10, 2026 07:03
Signed-off-by: Piotr Skamruk <piotr.skamruk@gmail.com>
@github-actions

Copy link
Copy Markdown

Merging this branch will decrease overall coverage

Impacted Packages Coverage Δ 🤖
shoot-grafter/controller/shoot 78.83% (-0.26%) 👎

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
shoot-grafter/controller/shoot/shoot_controller.go 83.69% (-0.93%) 141 (+11) 118 (+8) 23 (+3) 👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • shoot-grafter/controller/shoot/shoot_controller_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEAT shoot-grafter] - Delete clusters when Shoot is deleted

2 participants