#pr-reviews (2023-02)

Pull Request Reviews for Cloud Posse Projects

2023-02-01

2023-02-02

2023-02-03

2023-02-22

Erik Weber avatar
Erik Weber

Any chance this could get a quick review? https://github.com/cloudposse/terraform-aws-iam-s3-user/pull/52

#52 Add permission boundary as variable and pass to downstream module

what

• Adds the possibility to pass a permission boundary to the created user

why

• We, amongst others, require system users to have a permission boundary attached to them. This is to prevent accidental (or intended) too wide permission sets. • The downstream iam-system-user module has a variable for it, this commit adds the same variable and passes it down

1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

approved and merged, thanks

#52 Add permission boundary as variable and pass to downstream module

what

• Adds the possibility to pass a permission boundary to the created user

why

• We, amongst others, require system users to have a permission boundary attached to them. This is to prevent accidental (or intended) too wide permission sets. • The downstream iam-system-user module has a variable for it, this commit adds the same variable and passes it down

Erik Weber avatar
Erik Weber

Thank you

2023-02-23

2023-02-25

Nitin avatar
#159 Support for Activity Stream added

what

• Allow to create Activity Stream for RDS Cluster

why

• Can be used to enable and activity stream from the cluster module itself

1

2023-02-27

Paula avatar

Hi! im using this module https://github.com/cloudposse/terraform-aws-ecs-codepipeline/blob/master/main.tf and i need one extra stage before the build stage. Is worthy opening a PR adding a new variable called “pre-build-stages” which is a list of objects, and add it using “dynamic”? im trying to avoid to build my own module

jose.amengual avatar
jose.amengual

PRs are alway welcome

jose.amengual avatar
jose.amengual

just need to be patient to wait for a review

1

2023-02-28

Joe Niland avatar
Joe Niland
#194 Use map for task & task exec policy arns variables

what

• Replaced variables task_policy_arns and task_exec_policy_arns with task_policy_arns_map and task_exec_policy_arns_map respectively • Existing variables were moved to [variables-deprecated.tf](http://variables-deprecated.tf) and values will be internally converted to a map if variables are defined

why

• The for_each change implemented in 14008fc has the potential to cause the Terraform ‘“for_each” value depends on resource attributes that cannot be determined until apply’ error. • Modifying this input to use a map can circumvent this error

references

• closes #191

1
Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)

thanks! @Gocho would you have time to check if this patch fixes your issue?

#194 Use map for task & task exec policy arns variables

what

• Replaced variables task_policy_arns and task_exec_policy_arns with task_policy_arns_map and task_exec_policy_arns_map respectively • Existing variables were moved to [variables-deprecated.tf](http://variables-deprecated.tf) and values will be internally converted to a map if variables are defined

why

• The for_each change implemented in 14008fc has the potential to cause the Terraform ‘“for_each” value depends on resource attributes that cannot be determined until apply’ error. • Modifying this input to use a map can circumvent this error

references

• closes #191

Gocho avatar

Sure

1
Gocho avatar

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/194#issuecomment-1450508689

I only tested the behaviour I opened the issue for, not for all others commit included in this branch. Note that I also, for the test, tried the modification on my original, more complex, stack and it didn’t seem to cause any issue.

Comment on #194 Use map for task & task exec policy arns variables

Tested against my dummy stack in the issue.
Where it failed with

  source                    = "cloudposse/ecs-alb-service-task/aws"
  version                   = "0.66.4"

It now flawlessly works with

source = "git::<ssh://[email protected]/joe-niland/terraform-aws-ecs-alb-service-task.git?ref=8fc8878>"

Note that I didn’t do extensive tests on other modifications, I just reran the dummy stack + tested on original stack (more complex with more containers) that also succeeded.

1
Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)

Thanks a lot, this got released in 0.67.1

1
    keyboard_arrow_up