#pr-reviews (2023-08)

Pull Request Reviews for Cloud Posse Projects

2023-08-04

David Lozano avatar
David Lozano
#166 fix: add port mapping name (#162)

what

• Add optional variable name to portMappings to allow Service Discovery registration

why

• Unable to allow ECS container definitions to register with Service Discovery

references

• closes #162

Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)

please rebuild the readme, but otherwise looks good

#166 fix: add port mapping name (#162)

what

• Add optional variable name to portMappings to allow Service Discovery registration

why

• Unable to allow ECS container definitions to register with Service Discovery

references

• closes #162

2023-08-05

2023-08-08

Nitin avatar
#106 feat: add organizations as readonly access

what

• Add the ability to have organizations as trustees (read-only) for the ECR repository

why

• As described in #82 , it’s sometimes useful to allow an entire organization to consume images from a centralized repository

references

• closes #82

Nitin avatar
#111 Update main.tf

permission restrications for lambda only

what why references

2023-08-22

z0rc3r avatar
#56 fix: sg arn output

what

• When you’re using external sg with setting create_security_group = false and don’t want to create default one which is included in module you get response with nullable element and can’t concatenate null values.

│ Error: Invalid function argument
│ 
│   on .terraform/modules/elasticache_memcached/outputs.tf line 12, in output "security_group_arn":
│   12:   value       = join("", module.aws_security_group[*].arn)
│     ├────────────────
│     │ while calling join(separator, lists...)
│     │ module.aws_security_group is object with 4 attributes
│ 
│ Invalid value for "lists" parameter: element 0 is null; cannot concatenate null values.
mbarrien avatar
mbarrien

Going to re-request a review on https://sweetops.slack.com/archives/CUJPCP1K6/p1690182346767379, my org needs this too.

1
Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

We were hoping to overhaul this module by now, which would have included this feature and more, but obviously we have not gotten to it yet.

I requested changes via the PR.

Dominique Dumont avatar
Dominique Dumont

I’ve updated this PR to address all the comments. I hope it will be reviewed soon.

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

@Dominique Dumont This is failing the Terratest. I’m not sure why, something to do with order of operations or waiting. You can compare with our s3-bucket module. It might be a while before I get to look into this further.

│ Error: creating S3 Bucket (<bucket name>) Versioning: operation error S3: PutBucketVersioning, https response error StatusCode: 409, RequestID: 05DHXWG91G2TNQ1Z, HostID: 68w0sDw58YSsDXCrVGYO9VYCJf3laQ0qZevy8seqI0vLOhF/96W+6jstaTFTBqusmEKVd0vul5FUDFQHR1FpdQ==, api error OperationAborted: A conflicting conditional operation is currently in progress against this resource. Please try again.
│ 
│   with module.tfstate_backend.aws_s3_bucket_versioning.default[0],
│   on ../../main.tf line 190, in resource "aws_s3_bucket_versioning" "default":
│  190: resource "aws_s3_bucket_versioning" "default" {
cloudposse/terraform-aws-s3-bucket

Terraform module that creates an S3 bucket with an optional IAM user for external CI/CD systems

Dominique Dumont avatar
Dominique Dumont

Thanks for the notice. I’ll look into this next week.

Dominique Dumont avatar
Dominique Dumont

@Jeremy G (Cloud Posse) This test is now marked as successful. I guess this was a temporary failure. Could you approve this PR ?

2023-08-23

Quentin BERTRAND avatar
Quentin BERTRAND

Hello there, I’ve (finally) fixed that PR; https://github.com/cloudposse/terraform-aws-nlb/pull/33

#33 Feat: add option to not create target group

what

Add option to not create default target group

why

Default target group can be useless.

references

#22

I guess it will need a procedure migration of state is this PR is accepted.

1
Gabriela Campana (Cloud Posse) avatar
Gabriela Campana (Cloud Posse)

@Igor Rodionov @Dan Miller (Cloud Posse)

#33 Feat: add option to not create target group

what

Add option to not create default target group

why

Default target group can be useless.

references

#22

I guess it will need a procedure migration of state is this PR is accepted.

1
1

2023-08-24

2023-08-28

    keyboard_arrow_up