Add acm certificate lifecycle create before destroy rule #202
Conversation
| // NewAwsAcmCertificateLifecycleRule returns new rule with default attributes | ||
| func NewAwsAcmCertificateLifecycleRule() *AwsAcmCertificateLifecycleRule { | ||
| return &AwsAcmCertificateLifecycleRule{ | ||
| // TODO: Write resource type and attribute name here |
There was a problem hiding this comment.
Please remove all TODO comments.
|
|
||
| // Severity returns the rule severity | ||
| func (r *AwsAcmCertificateLifecycleRule) Severity() string { | ||
| return tflint.ERROR |
There was a problem hiding this comment.
This rule is about best practices, so I think warning is appropriate for severity.
| func (r *AwsAcmCertificateLifecycleRule) Check(runner tflint.Runner) error { | ||
| return runner.WalkResources("aws_acm_certificate", func(resource *configs.Resource) error { | ||
| if !resource.Managed.CreateBeforeDestroy { | ||
| if err := runner.EmitIssue(r, "lifecycle {\n create_before_destroy = true\n} needs to be set for `aws_acm_certificate`", resource.DeclRange); err != nil { |
There was a problem hiding this comment.
This error message is good to understand, but personally, I would like to avoid messages containing newlines as much as possible. This is because some formatters (e.g. -f compact) that are supposed to be parsed by regexp may be broken.
| return project.ReferenceLink(r.Name()) | ||
| } | ||
|
|
||
| // Check checks ... |
There was a problem hiding this comment.
Please edit the comment about this method.
| End: hcl.Pos{Line: 2, Column: 38}, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
It would be better to have a test case with create_before_destroy = true.
| @@ -0,0 +1,49 @@ | |||
| # aws_acm_certificate_lifecycle | |||
There was a problem hiding this comment.
Please add a link to the document index.
https://github.com/terraform-linters/tflint-ruleset-aws/blob/master/docs/rules/README.md
|
Thanks for the feedback, I'll fix all the stuff you mentioned shortly |
AleksaC
force-pushed
the
acm-certificate-lifecycle
branch
2 times, most recently
from
035a48e to
1885874
Compare
|
Everything should be fixed now. I also spotted two broken links in the docs so I fixed them as well. Let me know if you wish me to move them to a separate pull request. |
| @@ -48,6 +48,7 @@ These rules enforce best practices and naming conventions: | |||
|
|
|||
| |Rule|Description|Enabled by default| | |||
| | --- | --- | --- | | |||
| |[aws_acm_certificate_lifecycle](aws_acm_certificate_lifecycle.md)|Disallow adding `aws_acm_certificate` resource without setting `create_before_destroy = true` in `lifecycle` block |✔| | |||
There was a problem hiding this comment.
Sorry for the lack of explanation. Please fix docs/rules/README.md.tmpl instead, and run go generate ./.... The build looks failed.
https://github.com/terraform-linters/tflint-ruleset-aws/blob/c34979e4c88c750ff97b302ce56209bdc70f0b46/docs/rules/README.md.tmpl
https://github.com/terraform-linters/tflint-ruleset-aws/runs/4384564812?check_suite_focus=true
|
|
||
| Error: lifecycle { | ||
| create_before_destroy = true | ||
| } needs to be set for `aws_acm_certificate` (aws_acm_certificate_lifecycle) |
There was a problem hiding this comment.
It would be nice if you could change this message as well.
AleksaC
force-pushed
the
acm-certificate-lifecycle
branch
from
1885874 to
7c94597
Compare
|
It's fixed now. I had trouble running go generate since aws sdk submodule wasn't included and my editor was showing it as an empty directory so I didn't realize it was a submodule until I looked at it on github. |
Official docs for
aws_acm_certificaterecommend settingcreate_before_destroy = truein lifecycle block of this resource since without doing this terraform will be unable to delete a certificate that's in use when trying to replace it and will timeout. I had this problem a few days ago and thought it would be a useful rule to lint.