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

Skip destroy if state is empty #80

Merged
merged 4 commits into from
Feb 24, 2021
Merged

Skip destroy if state is empty #80

merged 4 commits into from
Feb 24, 2021

Conversation

timuthy
Copy link
Contributor

@timuthy timuthy commented Feb 23, 2021

How to categorize this PR?

/area dev-productivity
/area usability
/kind enhancement
/priority normal

What this PR does / why we need it:
If Terraform never ran successfully because of a broken configuration, it'll also fail during the destroy step as described in the linked issue. We can skip further execution in such cases if the state is empty and just remove the finalizer from the state ConfigMap.

Which issue(s) this PR fixes:
Fixes #78

Special notes for your reviewer:
There are other cases in which the state is empty in terms of the Terraform object structure can be found in the state ConfigMap but resources are empty:

  terraform.tfstate: |
    {
      "version": 4,
      "terraform_version": "0.12.29",
      "serial": 62,
      "lineage": "",
      "outputs": {},
      "resources": []
    }

I decided not take a shortcut here because we'd need to explicitly parse the structure and thus introduce a version dependency to Terraform. Still, Terraform will handle such cases gracefully i.e., the only overhead is that we might run Terraform destroy even though it's not necessary (/cc @timebertt).

Along the way I updated:

  • Go dependencies
  • Go version to 1.16

Release note:

The Terraformer now instantly removes its finalizer from the state `ConfigMap` if the state is empty and `destroy` is called. A separate Terraform `destroy` is not executed.

@timuthy timuthy requested a review from a team as a code owner February 23, 2021 17:15
@gardener-robot gardener-robot added the needs/review Needs review label Feb 23, 2021
@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 Feb 23, 2021
@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) area/usability Usability related kind/enhancement Enhancement, improvement, extension priority/normal size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Feb 23, 2021
@timuthy
Copy link
Contributor Author

timuthy commented Feb 23, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Feb 23, 2021

Testrun: e2e-2wjl5
Workflow: e2e-2wjl5-wf
Phase: Failed

+--------------+--------------+--------+----------+
|     NAME     |     STEP     | PHASE  | DURATION |
+--------------+--------------+--------+----------+
| pod-e2e-test | pod-e2e-test | Failed | 12m2s    |
+--------------+--------------+--------+----------+

@gardener-robot-ci-1 gardener-robot-ci-1 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 Feb 23, 2021
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

Thanks!
I have some minor nits.

go.mod Outdated Show resolved Hide resolved
pkg/terraformer/terraformer.go Outdated Show resolved Hide resolved
pkg/terraformer/terraformer_test.go Show resolved Hide resolved
pkg/terraformer/terraformer.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 23, 2021
@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 Feb 24, 2021
timebertt
timebertt previously approved these changes Feb 24, 2021
Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Very nice!
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Feb 24, 2021
@timebertt
Copy link
Member

I decided not take a shortcut here because we'd need to explicitly parse the structure and thus introduce a version dependency to Terraform. Still, Terraform will handle such cases gracefully i.e., the only overhead is that we might run Terraform destroy even though it's not necessary (/cc @timebertt).

Sounds reasonable. Great!

@timuthy
Copy link
Contributor Author

timuthy commented Feb 24, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Feb 24, 2021

Testrun: e2e-gxhrj
Workflow: e2e-gxhrj-wf
Phase: Failed

+--------------+--------------+--------+----------+
|     NAME     |     STEP     | PHASE  | DURATION |
+--------------+--------------+--------+----------+
| pod-e2e-test | pod-e2e-test | Failed | 11m47s   |
+--------------+--------------+--------+----------+

@timebertt
Copy link
Member

/test

FYI: the test can only run successfully after the PR build has finished, as it uses the image built by the PR job.

@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 Feb 24, 2021
vpnachev
vpnachev previously approved these changes Feb 24, 2021
Copy link
Member

@vpnachev vpnachev 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 needs/review Needs review and removed needs/review Needs review labels Feb 24, 2021
@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 Feb 24, 2021
@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Feb 24, 2021
@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 Feb 24, 2021
@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 Feb 24, 2021
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Feb 24, 2021
@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 Feb 24, 2021
@gardener-robot gardener-robot removed the needs/rebase Needs git rebase label Feb 24, 2021
@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 Feb 24, 2021
@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 Feb 24, 2021
@timuthy
Copy link
Contributor Author

timuthy commented Feb 24, 2021

/test

@testmachinery
Copy link

testmachinery bot commented Feb 24, 2021

Testrun: e2e-k9xpd
Workflow: e2e-k9xpd-wf
Phase: Succeeded

+--------------+--------------+-----------+----------+
|     NAME     |     STEP     |   PHASE   | DURATION |
+--------------+--------------+-----------+----------+
| pod-e2e-test | pod-e2e-test | Succeeded | 2m51s    |
+--------------+--------------+-----------+----------+

@timuthy
Copy link
Contributor Author

timuthy commented Feb 24, 2021

@vpnachev, @timebertt thanks for your reviews. Another commit was necessary for a successful verify execution. PTAL.

Copy link
Member

@timebertt timebertt 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 reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/second-opinion Needs second review by someone else labels Feb 24, 2021
@timuthy timuthy merged commit e01606c into master Feb 24, 2021
@timuthy timuthy deleted the enhancement.destroy branch February 24, 2021 13:27
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev-productivity Developer productivity related (how to improve development) area/usability Usability related kind/enhancement Enhancement, improvement, extension kind/test Test needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/l Size of pull request is large (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to delete Shoot with wrong floatingPoolName
7 participants