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

fix: cleans up principals lambda logic to separate policy doc #105

Merged
merged 5 commits into from
May 26, 2023

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented May 19, 2023

what

  • Clean up of the logic surrounding the var.principals_lambda policies

why

  • When this was originally implemented it was copy / pastad across multiple policy docs, which isn't necessary and creates a bunch of bloat.

references

@Gowiem Gowiem requested review from a team as code owners May 19, 2023 04:03
@Gowiem Gowiem requested a review from dotCipher May 19, 2023 04:03
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member Author

Gowiem commented May 19, 2023

/terratest

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Gowiem
Copy link
Member Author

Gowiem commented May 19, 2023

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented May 24, 2023

/terratest

main.tf Outdated
}

data "aws_iam_policy_document" "lambda_access" {
count = module.this.enabled ? 1 : 0
Copy link
Member

Choose a reason for hiding this comment

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

if var.principals_lambda is empty, do we need to create this aws_iam_policy_document at all?
condition.values will be an empty list

Copy link
Member Author

Choose a reason for hiding this comment

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

@aknysh yeah, this is how the other policy docs are built, so I was just staying with the pattern that is already established + decrease the risk of breaking something. Think we should rework?

Copy link
Member

Choose a reason for hiding this comment

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

let's have it as it was before.
We can update it in a separate PR

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.

@Gowiem LGTM, please see comments

@Gowiem Gowiem requested a review from aknysh May 24, 2023 21:57
main.tf Outdated Show resolved Hide resolved
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.

please see comments

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.

please see comments

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@Gowiem
Copy link
Member Author

Gowiem commented May 25, 2023

/terratest

@Gowiem Gowiem enabled auto-merge (squash) May 25, 2023 04:06
@Gowiem Gowiem requested a review from aknysh May 25, 2023 04:06
@Gowiem Gowiem merged commit 862fc85 into main May 26, 2023
11 checks passed
@Gowiem Gowiem deleted the fix/principals-lambda-policy branch May 26, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants