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

aws | missing launch template #4579

Merged
merged 15 commits into from Aug 27, 2019
Merged

aws | missing launch template #4579

merged 15 commits into from Aug 27, 2019

Conversation

PratMis
Copy link
Collaborator

@PratMis PratMis commented Aug 12, 2019

Closes #4568

Added a try/except block around describe_launch_template_versions api call. Continues if the launch template is not found and raises in all other cases

@PratMis PratMis changed the title Lt missing aws | missing launch template Aug 12, 2019
@PratMis
Copy link
Collaborator Author

PratMis commented Aug 12, 2019

Might have to put in a test case I guess. If this looks good @kapilt, I can get the tests in! Thanks

@kapilt
Copy link
Collaborator

kapilt commented Aug 13, 2019

approach looks fine wrt to api exception, but there's some additional behavior validation needed around the filter itself, ie if attribute equality check on launch template that doesn't exist shouldn't return results for non existent. also brings up the another question of how do you check for an asg with a non existent launch template version, ideally that would be handled by the invalid asg filter check but the correctness depends in part on behavior here I suspect.

@PratMis
Copy link
Collaborator Author

PratMis commented Aug 13, 2019

Cool, I'll try to add those changes in! Thanks

@PratMis
Copy link
Collaborator Author

PratMis commented Aug 13, 2019

Hello @kapilt! I have added some code:
Added an exception handler for non existent launch template id version
Looking into the invalid filter, had missed your edited comment

@kapilt
Copy link
Collaborator

kapilt commented Aug 22, 2019

needs some additional test coverage on the exception handling behavior, if you need an example of injecting clients with failures, patch or mock is appropriate, there are other resource tests doing the same.

@PratMis
Copy link
Collaborator Author

PratMis commented Aug 22, 2019

Thanks for your input! I'll get the test cases out such that the codecov goes up. Right now looks like its not respecting the test I wrote as per your suggestion.
Also, I looked into the invalid filter. From what I understand, we can return an ASG as invalid if the launch template/launch-template-version it is tied to doesn't exist. But that wouldn't help in this case as it would make the logic redundant.

Update: For testing, used two (lt, lt-version) tuples. One of them doesn't exist. Passed the arguments through get_resources() in class LaunchTemplate. Even if one of them doesn't exist, the exception was handled and the results returned as expected. Thanks for your help!

@PratMis PratMis requested a review from kapilt August 23, 2019 18:29
Copy link
Collaborator

@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, lgtm

@kapilt kapilt merged commit 5f12e81 into cloud-custodian:master Aug 27, 2019
@PratMis PratMis deleted the lt-missing branch August 27, 2019 16:58
fidelito pushed a commit to fidelito/cloud-custodian that referenced this pull request May 29, 2020
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.

aws - ami unused with missing launch template
2 participants