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

Expose variable s3_object_ownership #67

Merged

Conversation

adamantike
Copy link
Contributor

what

  • Add s3_object_ownership variable for passthrough to the cloudposse/s3-log-storage/aws module.
  • Bump minimum Terraform version to 1.3.0, which is already required by the underlying module cloudposse/s3-log-storage/aws (reference).

why

Terraform code using this module could require different values than the default, for s3_object_ownership.

Specifically, this will allow us to fix cloudposse/terraform-aws-ecs-web-app#225, which is currently failing when trying to create ACLs, with error:

AccessControlListNotSupported: The bucket does not allow ACLs

Terraform code using this module could require different values than the
default, for `s3_object_ownership`.

Specifically, this will allow us to fix
cloudposse/terraform-aws-ecs-web-app#225, which
is currently failing when trying to create ACLs, with error:

> AccessControlListNotSupported: The bucket does not allow ACLs
Makefile Outdated
@@ -1,4 +1,5 @@
SHELL := /bin/bash
export TERRAFORM_VERSION = 1.3.9
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to pin this version here? I haven't seen this elsewhere, so I'm inclined to say we shouldn't do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't make make lint pass without this change, as build-harness downloads Terraform 1.0 by default (related code).

I saw this implemented here: https://github.com/cloudposse/terraform-aws-s3-bucket/blob/d3c353f3170347352684b04a75cb62d4bc91ba6e/Makefile#L2

However, I don't think CI runs make lint? As https://github.com/cloudposse/terraform-aws-s3-log-storage doesn't include this line, but it requires Terraform >= 1.3.0, running make lint in that project also fails.

@Gowiem Gowiem requested a review from Nuru May 29, 2023 14:31
@Gowiem
Copy link
Member

Gowiem commented May 29, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented May 29, 2023

Pulling in @Nuru on this PR as I know he was the one who untangled the S3 bucket ACL / policy changes in our most basic S3 child module. Figured he should weigh in here on how changes like this should proceed.

@hans-d hans-d added the stale This PR has gone stale label Mar 2, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

@Nuru any update re this ?

@hans-d hans-d requested a review from Gowiem March 3, 2024 12:45
@hans-d
Copy link

hans-d commented Mar 3, 2024

@Gowiem can you also have a recheck re this pr to get it moving again?

@hans-d
Copy link

hans-d commented Mar 3, 2024

@adamantike Hi, can you update the pr so that it passes the tests? otherwise, it is likely to be closed due to staleness.

@mergify mergify bot removed the stale This PR has gone stale label Apr 1, 2024
Copy link

mergify bot commented Apr 1, 2024

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Apr 1, 2024
@goruha
Copy link
Member

goruha commented May 3, 2024

/terratest

Nuru
Nuru previously approved these changes May 6, 2024
@Nuru
Copy link
Contributor

Nuru commented May 6, 2024

/terratest

@Nuru
Copy link
Contributor

Nuru commented May 6, 2024

/terratest

@Nuru Nuru added the no-release Do not create a new release (wait for additional code changes) label May 6, 2024
@Nuru Nuru self-requested a review May 6, 2024 08:27
@Nuru Nuru merged commit ae8f6e9 into cloudposse:main May 6, 2024
17 checks passed
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label May 6, 2024
@Nuru
Copy link
Contributor

Nuru commented May 6, 2024

Released in v0.20.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants