#pr-reviews (2022-03)

Pull Request Reviews for Cloud Posse Projects

2022-03-02

kevcube avatar
kevcube

Pin AWS Provider to 3.x on LB Logs Bucket module

https://github.com/cloudposse/terraform-aws-alb/pull/110

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

Ty for the contribution but we do not pin modules to major versions.

kevcube avatar
kevcube

@RB (Ronak) (Cloud Posse) I updated the PR to instead lock to less-than-4 because the module breaks when used with AWS Provider v4. Sorry that I didn’t include more context in the original. PR

kevcube avatar
kevcube

what

• Version lock AWS provider to less-than-4

why

• Provider version 4 breaks this module because of the downstream alb logs bucket

kevcube avatar
kevcube
kevcube avatar
kevcube

Once aws-ecs-codepipeline is merged, I will also be asking for aws-ecs-web-app to be updated to point to that new version

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

@kevcube, we make it a point to not pin major aws versions

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

the way forward would be to move the s3 bucket in the ecs code pipeline to use the s3 bucket module

https://github.com/cloudposse/terraform-aws-ecs-codepipeline/blob/882d7e0ea01e932bdc70ada8c953ec5bc7df361f/main.tf#L9

resource "aws_s3_bucket" "default" {
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

then we can update the pin for ecs web module to >= 4.0

kevcube avatar
kevcube

Ok I understand now. Unfortunately this process will introduce breaking changes for users of this module, which makes me worry that these PRs will move along much slower. I wish that in the meantime we could lock things to provider version less-than-4

2022-03-03

2022-03-21

Brian Ojeda avatar
Brian Ojeda

what

• add new scp to prevent cloudtrail from being disabled

why

• prevent cloudtrail from being disabled by bad actors

references

• n/a

Brian Ojeda avatar
Brian Ojeda

Updated the PR. It had a misplace comma.

what

• add new scp to prevent cloudtrail from being disabled

why

• prevent cloudtrail from being disabled by bad actors

references

• n/a

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

thought we had one for this

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@jose.amengual

1

2022-03-23

2022-03-28

Bart Coddens avatar
Bart Coddens

Hi all, what needs to be done to get this integrated ?

Bart Coddens avatar
Bart Coddens

what

• Upgrade to support AWS provider v4 • Upgrade to latest s3 bucket module

why

• This module is currently unusable in projects using AWS provider v4

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

cc: @Jeremy G (Cloud Posse) i approved this. tests passed and the versions for terraform match s3 bucket module except s3 bucket module pins aws provider to 4.2.x and this repo pr pins to 4.x which is minor

what

• Upgrade to support AWS provider v4 • Upgrade to latest s3 bucket module

why

• This module is currently unusable in projects using AWS provider v4

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

should be safe to merge unless we also want to include migration docs?

Bart Coddens avatar
Bart Coddens

thx guys

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

@RB (Ronak) (Cloud Posse) @Bart Coddens We need migration docs. You can reference the S3 bucket migration doc (do not need to completely duplicated it), but you need to explain how it applies to this module.

I added something minimal to the release notes, but since I don’t know what the experience is of upgrading, I cannot tell if it is sufficient or not.

Bart Coddens avatar
Bart Coddens

I am interested in this as well

    keyboard_arrow_up