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

Allow creating a policy for an existing S3 bucket #154

Merged
merged 3 commits into from
Apr 26, 2021
Merged

Allow creating a policy for an existing S3 bucket #154

merged 3 commits into from
Apr 26, 2021

Conversation

emyller
Copy link
Contributor

@emyller emyller commented Apr 19, 2021

what

  • Fix creating a S3 bucket policy when using an existing S3 bucket.

why

  • The code was only considering the bucket object managed by this module.

references

@emyller emyller requested review from a team as code owners April 19, 2021 20:09
@emyller emyller changed the title Allow creating a policy for an existing S3 bucket (#152) Allow creating a policy for an existing S3 bucket Apr 19, 2021
nitrocode
nitrocode previously approved these changes Apr 20, 2021
@nitrocode
Copy link
Member

/test all

@emyller
Copy link
Contributor Author

emyller commented Apr 20, 2021

@nitrocode I can't make complete sense out of the errors pointed by CI. Would you give me a hint at how to fix these?

@nitrocode
Copy link
Member

Try the following to kick off the build

git commit --allow-empty
git push -f

@emyller
Copy link
Contributor Author

emyller commented Apr 21, 2021

@nitrocode No luck. The log complains it can't push to my repo, or so it looks like:

+ git push
remote: Permission to continuumvip/terraform-aws-cloudfront-s3-cdn.git denied to cloudpossebot.
fatal: unable to access 'https://github.com/continuumvip/terraform-aws-cloudfront-s3-cdn/': The requested URL returned error: 403

Should the bot really be trying to do that?

@nitrocode
Copy link
Member

Yes it should as that's how the Auto Format works. It clones your repo, checks if formatting is necessary, and then pushes up any changes.

You can format the code yourself and it will bypass that bot if you like or you can give your fork permission to the cloudposse bot.

@emyller
Copy link
Contributor Author

emyller commented Apr 21, 2021

(...) or you can give your fork permission to the cloudposse bot.

Cool, that makes sense. I have invited the bot to the repository as a contributor, it's pending the invite. Can you please accept it and let me know so we can try triggering CI again? Thanks!

@pearsonhenri
Copy link

Anything I can do to help on this PR? Would looove to pull these changes in :)

@emyller
Copy link
Contributor Author

emyller commented Apr 26, 2021

@pearsonhenri I think we need Cloudposse to accept the invite to their bot account so it can push to the fork, or a volunteer to set up their linting environment locally and contribute with the necessary changes -- sorry I don't have time to do the latter. Right now this is basically how I'm using this module and it's fine:

module "my_cdn" {
  # source = "cloudposse/cloudfront-s3-cdn/aws"
  # version = "0.59.0"
  source = "github.com/continuumvip/terraform-aws-cloudfront-s3-cdn?ref=2f24a73"
  ...
}

Gowiem
Gowiem previously approved these changes Apr 26, 2021
@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

@emyller you can run the formatting code yourself by doing the following:

make init 
make pr/auto-format

Then commit your changes and push the result and that should get this passed that annoying auto-format bit.

Apologies for the delay, please ping me directly once you've done the above steps or post in #pr-reviews in the SweetOps Slack and somebody else will get to it.

@emyller emyller requested a review from a team as a code owner April 26, 2021 23:13
@emyller emyller requested review from jamengual and srhopkins and removed request for a team April 26, 2021 23:13
@emyller
Copy link
Contributor Author

emyller commented Apr 26, 2021

/test all

@emyller
Copy link
Contributor Author

emyller commented Apr 26, 2021

Then commit your changes and push the result and that should get this passed that annoying auto-format bit.

Thanks @Gowiem. Can you please re-add your review?

@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

/teset all

@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

/test all

@Gowiem Gowiem merged commit f6c3ce2 into cloudposse:master Apr 26, 2021
@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

Thanks for the contribution @emyller! Released as 0.62.0.

@pearsonhenri
Copy link

Yay thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to override policy of existing S3 bucket
4 participants