Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure remedy controller resources have been deleted #138

Merged

Conversation

stoyanr
Copy link
Contributor

@stoyanr stoyanr commented Jul 31, 2020

How to categorize this PR?
/area control-plane
/kind enhancement
/priority normal
/platform azure

What this PR does / why we need it:
Ensures that all remaining remedy controller resources have been deleted before deleting the remedy controller itself.

Which issue(s) this PR fixes:
Fixes gardener/remedy-controller#32

Special notes for your reviewer:

Release note:

The controlplane controller now ensures that all remaining remedy controller resources have been deleted before deleting the remedy controller itself.

@stoyanr stoyanr requested review from a team as code owners July 31, 2020 12:24
@gardener-robot gardener-robot added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension platform/azure Microsoft Azure platform/infrastructure priority/normal labels Jul 31, 2020
@gardener-robot-ci-1
Copy link
Contributor

Thank you @stoyanr for your contribution. I will start a build for your PR. Once started, the build URL will be posted here.

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@gardener-robot-ci-3
Copy link
Contributor

A build of this pull request has started. You can check on its progress here: https://concourse.ci.gardener.cloud/teams/gardener/pipelines/gardener-extension-provider-azure-master/jobs/master-pull-request-job/builds/3

@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 31, 2020
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from 71c920e to 4a95d03 Compare July 31, 2020 12:25
@gardener-robot-ci-3
Copy link
Contributor

Thank you @stoyanr for updating the PR. I will start a build. Once started, the build URL will be posted here.

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@gardener-robot-ci-1
Copy link
Contributor

A build of this pull request has started. You can check on its progress here: https://concourse.ci.gardener.cloud/teams/gardener/pipelines/gardener-extension-provider-azure-master/jobs/master-pull-request-job/builds/4

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@AndreasBurger
Copy link
Member

So if I understand this correctly, part of the Remedy Controller is now vendored in here so that we can enhance the extension-provider to also clean up after the Remedy Controller.
On the one hand, I don't like the coupling of both components. On the other, I think this a pragmatic approach.

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

PR looks great, no suggestions. Do you want to add some unit test for the deletion?

@rfranzke
Copy link
Member

@AndreasBurger The components are not coupled, or rather, what you mean with "coupled" is already in place without this PR. The Azure extension deploys the Azure remedy controller already with a specific version and a specific deployment manifest, so it already has knowledge about what the remedy controller will do.

@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from 4a95d03 to 531be59 Compare July 31, 2020 13:19
@gardener-robot-ci-3
Copy link
Contributor

Thank you @stoyanr for updating the PR. I will start a build. Once started, the build URL will be posted here.

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@gardener-robot-ci-1
Copy link
Contributor

A build of this pull request has started. You can check on its progress here: https://concourse.ci.gardener.cloud/teams/gardener/pipelines/gardener-extension-provider-azure-master/jobs/master-pull-request-job/builds/5

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from 531be59 to 23ff868 Compare July 31, 2020 13:38
@gardener-robot-ci-1
Copy link
Contributor

Thank you @stoyanr for updating the PR. I will start a build. Once started, the build URL will be posted here.

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@gardener-robot-ci-1
Copy link
Contributor

A build of this pull request has started. You can check on its progress here: https://concourse.ci.gardener.cloud/teams/gardener/pipelines/gardener-extension-provider-azure-master/jobs/master-pull-request-job/builds/6

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@stoyanr
Copy link
Contributor Author

stoyanr commented Jul 31, 2020

@rfranzke To add unit tests, I need to generate mocks for the Actuator interface in gardener/gardener, since such don't exist yet. I could do this, however this would mean we would need a new Gardener release with the mocks before we could release the new Azure extension. Do you think it's worth the trouble? If yes, I'll do it.

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from 20e117b to ee79e86 Compare August 3, 2020 07:40
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 3, 2020

Rebased on top of #141, dependency to https://github.com/stoyanr/gardener removed.

@stoyanr stoyanr marked this pull request as draft August 3, 2020 09:16
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from ee79e86 to e3ba364 Compare August 3, 2020 11:02
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from e3ba364 to deb29e7 Compare August 3, 2020 13:33
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 3, 2020
@stoyanr stoyanr marked this pull request as ready for review August 3, 2020 13:38
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 3, 2020

Rebased on top of master and removed "draft".

rfranzke
rfranzke previously approved these changes Aug 3, 2020
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Aug 3, 2020
@rfranzke
Copy link
Member

rfranzke commented Aug 3, 2020

/needs rebase

@gardener-robot gardener-robot added needs/rebase Needs git rebase and removed reviewed/lgtm Has approval for merging labels Aug 3, 2020
@stoyanr stoyanr force-pushed the ensure-remedy-controller-resources-deleted branch from deb29e7 to 59fff49 Compare August 3, 2020 14:07
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 3, 2020
@rfranzke rfranzke merged commit 4479fcb into gardener:master Aug 3, 2020
@stoyanr stoyanr deleted the ensure-remedy-controller-resources-deleted branch August 4, 2020 06:51
@gardener-robot gardener-robot added the priority/3 Priority (lower number equals higher priority) label Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase platform/azure Microsoft Azure platform/infrastructure priority/3 Priority (lower number equals higher priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shoot could not be deleted due to a PublicIPAddress resource that can't be deleted
7 participants