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

Delete partial resource created by invalid create test #521

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

anshikg
Copy link
Contributor

@anshikg anshikg commented Jul 31, 2020

*Issue #, if available:*#517

Description of changes: Adding a delete call to delete partially created resources.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@anshikg anshikg requested review from ammokhov and ugudip July 31, 2020 23:31
Copy link
Contributor

@LaikaN57 LaikaN57 left a comment

Choose a reason for hiding this comment

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

The return before finally was a bit confusing for new python devs but interesting functionality. Also I guess if the delete operation fails (due to delete code bugs) then test will provided and ERROR status instead of FAILURE which seems ok but sort of makes writing a DELETE handler a requirement of writing and testing a CREATE handler. Overall it looks good though.

@LaikaN57
Copy link
Contributor

LaikaN57 commented Aug 1, 2020

Or maybe for consistency we should be asserting as well like in the test fixture for all of the other tests? https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/contract/suite/handler_create.py#L32

@ammokhov
Copy link
Contributor

ammokhov commented Aug 1, 2020

The return before finally was a bit confusing for new python devs but interesting functionality. Also I guess if the delete operation fails (due to delete code bugs) then test will provided and ERROR status instead of FAILURE which seems ok but sort of makes writing a DELETE handler a requirement of writing and testing a CREATE handler. Overall it looks good though.

Good point, not sure if DeleteHandler must be a requirement for the test but We should definitely try to delete created resource to avoid unexpected billing.

Or maybe for consistency we should be asserting as well like in the test fixture for all of the other tests? https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/contract/suite/handler_create.py#L32

I think we could put finally as additional annotation and add that to all the necessary tests. In addition, I agree that we should be consistent and use call_and_assert

@anshikg anshikg merged commit f22e4ea into aws-cloudformation:master Aug 3, 2020
@anshikg anshikg deleted the cleanup branch October 28, 2020 02:54
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.

4 participants