-
Notifications
You must be signed in to change notification settings - Fork 63
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
docs: fix typos and description #719
Conversation
@@ -5,7 +5,7 @@ locals { | |||
} | |||
|
|||
control "acm_certificate_expires_30_days" { | |||
title = "ACM certificates should be set to expire within 30 days" | |||
title = "ACM certificates should not expire within 30 days" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdecat - We derived this from AWS Config rule, the language was adjusted based on its description.
IMO, AWS has lowered further to 14 days as default from 30 days; we need to revalidate and adjust this query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is just a matter of wording then.
Given the original description:
The rule is NON_COMPLIANT if your certificates are about to expire.
What about :
title = "ACM certificates should not expire within 30 days" | |
title = "ACM certificates will expire within 30 days" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually our titles describe the OK/good state, e.g., CloudFormation stacks outputs should not have any secrets, SQS queues should be configured with a dead-letter queue. So in this case, I think "ACM certificates should not expire within 30 days" is a better title.
Thanks @pdecat for the fixes! We'll release this in our next version most likely sometime next week. |
Checklist