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.

Sign up for GitHub

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

Jump to bottom

Added log-delivery-policy for s3-bucket component #698

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Added log-delivery-policy for s3-bucket component #698

wants to merge 6 commits into from

Conversation

zdmytriv
Copy link
Contributor

@zdmytriv zdmytriv commented May 25, 2023 •
edited

what

  • Added log-delivery-policy for s3-bucket component

why

references

@zdmytriv zdmytriv requested review from a team as code owners May 25, 2023 10:51
@osterman osterman requested review from Nuru and aknysh May 25, 2023 12:11
aknysh
aknysh reviewed May 25, 2023
View reviewed changes
modules/s3-bucket/main.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed May 25, 2023
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

LGTM, please see comments

@zdmytriv zdmytriv requested a review from aknysh May 25, 2023 13:41
Nuru
Nuru requested changes May 25, 2023
View reviewed changes
modules/s3-bucket/variables.tf
Comment on lines +402 to +403
Map of IAM policy statements to use in the bucket policy. Conflicts with `var.custom_policy_enabled` and `var.log_delivery_policy_enabled`.
It will be used if `var.custom_policy_enabled` and `var.log_delivery_policy_enabled` are set to `false`.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

This description is wrong, right? This will be used when var.custom_policy_enabled is true.

Copy link
Contributor Author

@zdmytriv zdmytriv May 25, 2023 •
edited

Choose a reason for hiding this comment

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

nope.

When custom_policy_enabled=true then policy from iam.tf this will be used.

When iam_policy_statements=true (and other 2 are false) then policy from iam-policy module will be used this

modules/s3-bucket/variables.tf Outdated Show resolved Hide resolved
@zdmytriv zdmytriv force-pushed the added-log-delivery-policy-for-s3-bucket-component branch from be616cd to 4822e28 Compare May 25, 2023 19:24
aknysh
aknysh reviewed May 25, 2023
View reviewed changes
modules/s3-bucket/iam.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed May 25, 2023
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

see comments

@zdmytriv
Copy link
Contributor Author

@Nuru please re-review

@zdmytriv zdmytriv requested a review from Nuru May 25, 2023 20:26
@zdmytriv zdmytriv marked this pull request as draft May 26, 2023 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknysh aknysh aknysh left review comments

@Nuru Nuru Awaiting requested review from Nuru

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zdmytriv @Nuru @aknysh