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

fix: Add context tags to the IAM resources #45

Merged
merged 2 commits into from Oct 11, 2023
Merged

fix: Add context tags to the IAM resources #45

merged 2 commits into from Oct 11, 2023

Conversation

natemccurdy
Copy link
Contributor

@natemccurdy natemccurdy commented Oct 6, 2023 •
edited

what

Add tags = module.this.tags to each of the IAM resources so that they use the tags determined by the null/label context or the tags input.

why

Prior to this, the aws_iam_role and the aws_iam_policy created by this module did not include any of the tags passed via tags or via context.

@natemccurdy natemccurdy requested review from a team as code owners October 6, 2023 22:55
@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

/terratest

Gowiem
Gowiem previously approved these changes Oct 9, 2023
View reviewed changes
@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

@natemccurdy Please run the following commands, commit, and push the changes:

make init
make github/init
make readme

@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

/terratest

Gowiem
Gowiem previously approved these changes Oct 9, 2023
View reviewed changes
@Gowiem Gowiem added the patch A minor, backward compatible change label Oct 9, 2023
Gowiem
Gowiem reviewed Oct 9, 2023
View reviewed changes
iam-role.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

@natemccurdy on both PRs, you're getting this failure with the README:

diff --git a/README.md b/README.md
index e2c38[11](https://github.com/cloudposse/terraform-aws-lambda-function/actions/runs/6459584281/job/17535684364?pr=45#step:6:12)..76c[12](https://github.com/cloudposse/terraform-aws-lambda-function/actions/runs/6459584281/job/17535684364?pr=45#step:6:13)07 100644
--- a/README.md
+++ b/README.md
@@ -[14](https://github.com/cloudposse/terraform-aws-lambda-function/actions/runs/6459584281/job/17535684364?pr=45#step:6:15)1,7 +141,7 @@ Available targets:
 
 | Name | Version |
 |------|---------|
-| <a name="provider_aws"></a> [aws](#provider\_aws) | 5.20.0 |
+| <a name="provider_aws"></a> [aws](#provider\_aws) | >= 3.0 |
 
 ## Modules
 
diff --git a/docs/terraform.md b/docs/terraform.md
index 9bcd6a8..8d423b4 100644
--- a/docs/terraform.md
+++ b/docs/terraform.md
@@ -10,7 +10,7 @@
 
 | Name | Version |
 |------|---------|
-| <a name="provider_aws"></a> [aws](#provider\_aws) | 5.[20](https://github.com/cloudposse/terraform-aws-lambda-function/actions/runs/6459584281/job/17535684364?pr=45#step:6:21).0 |
+| <a name="provider_aws"></a> [aws](#provider\_aws) | >= 3.0 |
 
 ## Modules
 
README.md is outdated. Please run the following commands locally and push the file:
  make init
  make readme

That shouldn't happen because make readme should use a specific version of tf-docs and be consistent... Do you have pre-commit installed globally maybe? If you can, check that out and if worst comes to worst, I'd change the README.yaml / README.md manually to address that.

@natemccurdy
fix: Add context tags to the IAM resources
b6dfccb 
Prior to this, the `aws_iam_role` and the `aws_iam_policy` created by this
module did not include any of the tags passed via `tags` or via `context`.

This fixes that problem by specifying `tags = module.this.tags` on each of
those resources so that they use the tags specified determined by the
null/label context.
@natemccurdy
Copy link
Contributor Author

natemccurdy commented Oct 9, 2023 •
edited

Do you have pre-commit installed globally maybe?

I do not have pre-commit installed globally.

I've fixed the docs manually as per your suggestion.

@natemccurdy
chore: update module boilerplate and docs
c804e1b 
```
make init
make github/init
make readme
```
@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

/terratest

@natemccurdy
Copy link
Contributor Author

@Gowiem Great, tests are passing. What are the next steps now? Looks like we need another approval?

@Gowiem
Copy link
Member

Gowiem commented Oct 9, 2023

@natemccurdy Yeah -- Need an org owner to approve. I've requested someone to do that in our contributor channel. Will follow up!

@natemccurdy natemccurdy mentioned this pull request Oct 11, 2023
Merged
aknysh
aknysh approved these changes Oct 11, 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.

thanks @natemccurdy

@aknysh aknysh merged commit 127b10b into cloudposse:main Oct 11, 2023
10 checks passed
@natemccurdy natemccurdy deleted the fix/iam_tags branch October 11, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gowiem Gowiem Gowiem approved these changes

@aknysh aknysh aknysh approved these changes

@dotCipher dotCipher Awaiting requested review from dotCipher dotCipher is a code owner automatically assigned from cloudposse/contributors

@woz5999 woz5999 Awaiting requested review from woz5999 woz5999 is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
patch A minor, backward compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@natemccurdy @Gowiem @aknysh