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

(WIP) Update cloudfront behaviors to behave like lists and allow for ordering #9167

Conversation

PayscaleNateW
Copy link
Contributor

#7253

  • Updated references to sets instead of lists
  • Updated tests so they pass
  • Successfully added ordered rules to cloudfront

However, some setbacks:
Still encountering a bug that maybe someone can help me with... I get an index out of bounds exception when I run terraform apply due to this line:

cloudfront_distribution_configuration_structure.go
ForwardedValues: expandForwardedValues(m["forwarded_values"].(*schema.Set).List()[0].(map[string]interface{})),

There may also be a bug in field_reader.go addrToSchema. I'm not sure that code is robust against crazy combinations of nested sets and lists.

TODO:

  1. Fix index out of bounds exception
  2. Write new tests for this behavior
  3. Update documentation about the new syntax for cache_behaviors
  4. If necessary, concoct some migration plan from sets to lists... Then again, seemed to maybe work fine just making the changes to the tf file? Maybe to be safe, we can keep support for the set in a deprecated way.

@PayscaleNateW
Copy link
Contributor Author

Well, tempted to close this... seems like you have it under control!

s := []interface{}{}
for _, v := range cbs.Items {
s = append(s, flattenCacheBehavior(v))
}
return schema.NewSet(cacheBehaviorHash, s)
//return schema.NewSet(cacheBehaviorHash, s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops should delete that commented out part

@radeksimko
Copy link
Member

radeksimko commented Oct 2, 2016

Hi @PayscaleNateW
would #7395 fix the bug you are experiencing?

@radeksimko radeksimko added bug waiting-response An issue/pull request is waiting for a response from the community provider/aws labels Oct 2, 2016
@PayscaleNateW
Copy link
Contributor Author

@radeksimko Actually, #7485 fixed it perfectly, thanks! I pulled down your commits. I had a few merge conflicts with #7395 so I didn't give too much effort to cherry-pick that one. But it looks like your fix is way more thorough. I definitely recommend merging those in

@PayscaleNateW
Copy link
Contributor Author

Now trying to write acceptance tests and I'm getting InvalidArgument: The S3 bucket that you specified for CloudFront logs doesn't exist: mylogs.6205123422105706112.s3.amazonaws.com

It doesn't seem like the logs bucket is defined anywhere in the mock configs? Also, when I do define one, there may be a race condition where the bucket doesn't get created in time for cloudfront to get it. Maybe Amazon just started doing that validity check?

I also notice that simply trying terraform apply using that as the config file has the same error. The s3 buckets get created, but not in time for cloudfront to use them.

@PayscaleNateW
Copy link
Contributor Author

PayscaleNateW commented Oct 3, 2016

I believe there is a bug there pertaining to the logging bucket not being previously defined. However, I was running into issues with my naming when I tried to fix it. The name of the bucket should be a name, for example mylogs., and then the logging_config should have its bucket as mylogs..s3.amazonaws.com

I think I fixed this in
#9175

@adamb0mb
Copy link

adamb0mb commented Oct 3, 2016

I'd love to see this or #7395 merged!

@spencerewall
Copy link

Echo @adamb0mb!

@PayscaleNateW PayscaleNateW mentioned this pull request Oct 12, 2016
@mitchellh mitchellh removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 1, 2016
@apparentlymart
Copy link
Contributor

Hello @PayscaleNateW, and thanks for working on this!

As part of the the Terraform 0.10 release earlier this year, all of the Terraform providers were moved to their own repositories in the terraform-providers GitHub organization, and removed from the Terraform Core repository.

Unfortunately due to the fact that new issues and pull requests are being opened constantly, it was not possible for the various provider maintainers to merge all outstanding pull requests before this split, and there is no automatic way to migrate a pull request to a new repository.

As a result, this pull request can sadly no longer be applied as-is, and so I'm going to close it.

If you or someone else has the time and motivation to apply same changes to the aws provider repository and open a new PR there, the maintainers of that provider should be able to review and merge it.

Thanks again for working on this, and sorry it was not able to be merged before the provider repository changes.

@ghost
Copy link

ghost commented Apr 6, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants