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_lambda_end_of_life #154

Merged
merged 5 commits into from
Aug 12, 2021

Conversation

Rihoj
Copy link
Contributor

@Rihoj Rihoj commented Aug 8, 2021

This should fix #30 and is based on Phase 2 of this list

@Rihoj
Copy link
Contributor Author

Rihoj commented Aug 8, 2021

I decided I should probably add an end of support warning as well. I am not sure if it should be an error instead since you can't create resources that are at end of support, but you can still update them.

@wata727
Copy link
Member

wata727 commented Aug 9, 2021

Nice! I think these are very useful rules. I have some suggestions.

First of all, what do you think about changing both severities to warning? A runtime that has reached EOL can certainly not be created or updated, but the existing function is clearly valid if it does not change.

If you agree with this suggestion, it's probably best to combine the rules into one and name it something like aws_lambda_function_deprecated_rumtime.

@Rihoj
Copy link
Contributor Author

Rihoj commented Aug 9, 2021

I see where you are coming from with it not being an error. Deep down in me cringes at the thought of just putting a warning though.

I see three use cases while using one of these runtimes:

  1. A user tries to create a function using a runtime that is at EoS or EoL. This should obviously be an error as it is not valid.
  2. A user tries to update a function that is at EoL. This should be an error as well as it's not allowed.
  3. A user tries is not updating or creating a function, but it exists and is past EoS or EoL. This is a warning as the terraform is still valid.

As a general rule would we want to take the lesser or greater severity when alerting the users?

With all that said. I do see your point, and if you still want me to I will make the requested changes. I just thought I would share my thoughts first.

@wata727
Copy link
Member

wata727 commented Aug 10, 2021

Thank you for sharing your thought. As you said, the case 1 and 2 are clearly errors. However, TFLint cannot control the severity depending on the changes, so we have to choose between warning and error.

In such cases, I design to minimize false positives as much as possible. TFLint is designed to prefer avoiding false positives over false negatives. In the use cases, putting a warning for the case 1 and 2 is a false negative, but I prefer to avoid putting an error for the case 3 as a false positive.

@Rihoj
Copy link
Contributor Author

Rihoj commented Aug 10, 2021

@wata727 Makes sense to me. I will get those updated when I can. thank you for hearing me out.

@Rihoj
Copy link
Contributor Author

Rihoj commented Aug 11, 2021

@wata727 Would we want a rule with a severity of INFO for runtimes that are set to be deprecated in say 30 days or so? Just to try to give warning?

@wata727
Copy link
Member

wata727 commented Aug 11, 2021

In my opinion, It's enough to issue a warning according to the runtime support policy in the AWS documentation.

Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Looks good. I will merge as soon as you update the branch and resolve the conflict.

@wata727 wata727 merged commit 20a1464 into terraform-linters:master Aug 12, 2021
@wata727
Copy link
Member

wata727 commented Aug 12, 2021

👍

@PatMyron
Copy link
Contributor

wonder if this rule could read directly from https://github.com/aws-cloudformation/cfn-lint/blob/main/src/cfnlint/data/AdditionalSpecs/LmbdRuntimeLifecycle.json so the list only needs to be maintained in one spot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Tracks Lambda runtime deprecations
3 participants