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

If apply fails, attempt to run destroy #58

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

baolsen
Copy link
Contributor

@baolsen baolsen commented Oct 21, 2022

closes #59

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: baolsen / name: Bjorn Olsen (a78f465)

@baolsen
Copy link
Contributor Author

baolsen commented Oct 21, 2022

/easycla

@@ -12,7 +12,7 @@ classifiers=[
readme = "README.md"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dependencies update is required for the current black hook to work.

# Try to destroy partially applied resources
self.destroy()
finally:
raise e from None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentionally ignore any errors from Destroy. In the test case, Destroy happens to fail for the same reason as the apply fails. In production scenarios, Destroy will normally work and destroy anything partially created by the apply step.

@@ -266,7 +273,8 @@ def parse_state(

for r in data.get("resources", ()):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If resources are not correctly applied, the list of instances may be empty.

local_file.foo
]
}
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simulate a partial apply:
1 TF apply creates some resources (the local file in this case)
2 TF apply fail on some unrelated resource

Unfortunately local_file resource wont fail if the local file exists already, it just overwrites. So I couldn't use it for the second step.
Instead, we force a failure using a data element to read a non existent file. The side effect of this is the destroy step also fails. But I couldn't find another way to simulate a scenario where a partial apply happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baolsen
Copy link
Contributor Author

baolsen commented Oct 21, 2022

@kapilt ready for review

@kapilt
Copy link
Contributor

kapilt commented Oct 21, 2022

thanks for the pull request and detailed commentary.

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Base: 92.81% // Head: 92.90% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (a78f465) compared to base (2c4e418).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   92.81%   92.90%   +0.09%     
==========================================
  Files           8        8              
  Lines         473      479       +6     
==========================================
+ Hits          439      445       +6     
  Misses         34       34              
Impacted Files Coverage Δ
pytest_terraform/tf.py 96.49% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@baolsen
Copy link
Contributor Author

baolsen commented Oct 28, 2022

@kapilt CI done

Copy link
Contributor

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

thanks, this looks great.

@kapilt kapilt merged commit 7870178 into cloud-custodian:main Dec 20, 2022
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.

If apply fails, some infrastructure may remain
2 participants