-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
feat: Added variable to control the create log group permission #514
feat: Added variable to control the create log group permission #514
Conversation
@@ -122,7 +122,7 @@ data "aws_iam_policy_document" "logs" { | |||
effect = "Allow" | |||
|
|||
actions = compact([ | |||
!var.use_existing_cloudwatch_log_group ? "logs:CreateLogGroup" : "", | |||
!var.use_existing_cloudwatch_log_group && var.attach_create_log_group_permission ? "logs:CreateLogGroup" : "", |
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.
If you want to reuse the existing cloudwatch log group (from previous runs), set use_existing_cloudwatch_log_group = true
.
I am not sure that we need to have an identical variable attach_create_log_group_permission
.
Right?
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.
The scenario I am dealing with is ephemeral environments. When I first create a new environment the cloudwatch log group does not exist. So I need use_existing_cloudwatch_log_group
set to false. When I destroy the environment, the log group may or may not persist. I then want to recreate that environment at a later date which, if the group exists, will fail.
use_existing_cloudwatch_log_group
only works if you know the CloudWatch group will be there, I do not.
As stated in the feature request, a work around would be to create the CloudWatch group separately. However, I would rather contain the logic to a single module.
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.
To be honest, since we provision the CloudWatch group anyway, having CreateLogGroup
on the Lambda seems redundant to me unless there is something I'm missing.
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.
@antonbabenko thoughts?
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.
Looks good to be merged. Thanks!
## [6.5.0](v6.4.0...v6.5.0) (2023-11-22) ### Features * Added variable to control the create log group permission ([#514](#514)) ([c173c27](c173c27))
This PR is included in version 6.5.0 🎉 |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
Adding a variable to control the create log group permission on the lambda role. Default for the new variable is to add the permission to the role.
Motivation and Context
Described in the following feature request:
Fixes #503
Breaking Changes
None, variable default will keep current behaviour the same.
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull requestTested running the example/complete module with init, plan, apply, destroy and inspected the iam roles to confirm that existing lambda roles still had the create log group permission and the new lambda role does not have it.