Make it optional to override the origin s3 policy

[dmattia]

When specifying var.origin_bucket, it can be nice to leave the existing bucket's policy as is.

As an example, I manage an s3 bucket that multiple CloudFront dists use as their origin (without paths, they just use the same code).

In this case, I do not want to restrict the bucket to only talk to a single CF dist, as this module does by default.

[aknysh]

@dmattia please rebase

[dmattia]

Just rebased @aknysh

[aknysh]

/codefresh run test

[aknysh]

thanks @dmattia
please see comments

[dmattia]

Oops, thanks for catching that! Updated and rebased @aknysh

[aknysh]

thanks again @dmattia
Just to confirm, when usinf an existing origin bucket, will this line work https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/67/files?file-filters%5B%5D=.tf#diff-7a370d8342e7203b805911c92454f0f4L169 - here it depends on the bucket which never created in this case

thanks

[dmattia]

yep, I have modules using this code already, both with and without an existing origin given and have had no problems

[aknysh]

/codefresh run test

[aknysh]

@dmattia
do we really need var.override_origin_bucket_policy?
If you specify var.origin_bucket, the policy will not be created as well as the bucket itself.
If you don't specify the origin bucket, we want to create it as well as the policy.

[dmattia]

I would prefer the approach you're talking about here, where we only manage the policy if we manage the bucket.

The reason I added the var is for backwards compatibility, as the current code in master will manage the policy even if the bucket isn't managed.

If you're okay with removing var.override_origin_bucket_policy, I'll gladly do it!

@aknysh

[aknysh]

count  = local.using_existing_origin && ! var.override_origin_bucket_policy ? 0 : 1

In the expression above, if the existing bucket is provided, the policy will not be created because local.using_existing_origin is true, so ! var.override_origin_bucket_policy is completely redundant

Please update the logic.
So yes, we want to manage the policy if we create the bucket.
And make it configurable to manage the policy or not when an existing bucket is provided.
(this is not the expression above does. It will never create the policy if an existing bucket is provided).

Thanks

[dmattia]

(Apologies for the delay, I was OOO).

Thanks for the correction! I updated the logic, not sure how I made that mistake the first time round

[aknysh]

@dmattia please see comments, thanks

[aknysh]

@dmattia please rebase and address the comments, thanks again

[dmattia]

@aknysh rebased and updated code

[aknysh]

/codefresh run test

[aknysh]

thanks @dmattia

please address the error:

TestExamplesComplete 2020-01-31T19:48:11Z command.go:121: Error: Reference to undeclared local value                                                                                      
TestExamplesComplete 2020-01-31T19:48:11Z command.go:121:                                                                                                                                 
TestExamplesComplete 2020-01-31T19:48:11Z command.go:121:   on ../../main.tf line 72, in resource "aws_s3_bucket_policy" "default":                                                       
TestExamplesComplete 2020-01-31T19:48:11Z command.go:121:   72:   count = ! local.use_existing_origin || var.override_origin_bucket_policy ? 1 : 0                                        
TestExamplesComplete 2020-01-31T19:48:11Z command.go:121:                                                                                                                                 
TestExamplesComplete 2020-01-31T19:48:11Z command.go:121: A local value with the name "use_existing_origin" has not been declared.                                                        

[aknysh]

also, please run terraform fmt

[dmattia]

Ran fmt, and fixed the var name so that validate passes

[maximmi]

/codefresh run test

[maximmi]

@dmattia can't make a fix because of permission denied to push changes to your fork

! [remote rejected] dmattia/s3_policy -> dmattia/s3_policy (permission denied)
error: failed to push some refs to '[email protected]:dmattia/terraform-aws-cloudfront-s3-cdn.git'

could you please check for permissions there?

[maximmi]

@dmattia see comments

[dmattia]

@maximmi I added you as a contributor to the repo

[maximmi]

/codefresh run test

[maximmi]

thanks @dmattia

comments were addessed