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

provider/fastly: upgrade go-fastly to fix #12910 #13648

Merged
merged 2 commits into from
Apr 18, 2017

Conversation

aerostitch
Copy link
Contributor

@aerostitch aerostitch commented Apr 13, 2017

Fixes: #12910

  • Added acceptance tests that reproduce the issue
  • Fix the issue

The current acceptance test reveals that error which matches the explanation provided in the issue:

$ TF_ACC=1 go test ./builtin/providers/fastly -v  -timeout 120m -run TestAccFastlyServiceV1_defaultTTL
=== RUN   TestAccFastlyServiceV1_defaultTTL
--- FAIL: TestAccFastlyServiceV1_defaultTTL (52.79s)
	testing.go:280: Step 2 error: Check failed: Check 3/4 error: fastly_service_v1.foo: Attribute 'default_ttl' expected "0", got "3400"
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/fastly	53.310s

@aerostitch
Copy link
Contributor Author

aerostitch commented Apr 14, 2017

The fact that 0 is ignored is due to the presence of omitempty in settings.go in github.com/sethvargo/go-fastly as 0 is considered as empty value for uint in this case.
But if you get rid of if, we end up resetting the default_ttl to the default value of 3600.

Now this other behavior is due to the old version of the github.com/ajg/form that is used in the go-fastly library. In the new version we could use the KeepZeros attribute to tell that we don't want to consider 0 as empty values (which is what happens in encodeValue in the github.com/ajg/form/encode.go file.

Anyone has an opinion about that?

@aerostitch
Copy link
Contributor Author

Seems it's been reported a while back in the go-fastly api: fastly/go-fastly#20
Should we keep this open to keep the acceptance tests?

@aerostitch
Copy link
Contributor Author

Proposed a fix upstream: fastly/go-fastly#41
If that gets merged, we'll have to update the dependencies to be able to integrate it.

@aerostitch
Copy link
Contributor Author

Will upgrade the go-fastly dependency in vendor/ this morning, test it for a little while and probably push the change here this afternoon.

@aerostitch aerostitch changed the title [WIP] provider/fastly: default-ttl not updating when set to 0 provider/fastly: upgrade go-fastly to fix #12910 Apr 17, 2017
@aerostitch
Copy link
Contributor Author

aerostitch commented Apr 17, 2017

Pushed the upgrade. Some dependencies needed to be upgraded as well.
Also changed the version usage in terraform to reflect the change from string to int.
All functional tests are passing now.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

TF_ACC=1 go test ./builtin/providers/fastly -v -run=TestAccFastlyServiceV1 -timeout 120m
=== RUN   TestAccFastlyServiceV1CacheSetting_basic
--- PASS: TestAccFastlyServiceV1CacheSetting_basic (36.08s)
=== RUN   TestAccFastlyServiceV1_conditional_basic
--- PASS: TestAccFastlyServiceV1_conditional_basic (15.33s)
=== RUN   TestAccFastlyServiceV1_gzips_basic
--- PASS: TestAccFastlyServiceV1_gzips_basic (34.15s)
=== RUN   TestAccFastlyServiceV1_headers_basic
--- PASS: TestAccFastlyServiceV1_headers_basic (34.97s)
=== RUN   TestAccFastlyServiceV1_healthcheck_basic
--- PASS: TestAccFastlyServiceV1_healthcheck_basic (37.57s)
=== RUN   TestAccFastlyServiceV1_papertrail_basic
--- PASS: TestAccFastlyServiceV1_papertrail_basic (34.44s)
=== RUN   TestAccFastlyServiceV1RequestSetting_basic
--- PASS: TestAccFastlyServiceV1RequestSetting_basic (15.47s)
=== RUN   TestAccFastlyServiceV1_response_object_basic
--- PASS: TestAccFastlyServiceV1_response_object_basic (36.13s)
=== RUN   TestAccFastlyServiceV1_s3logging_basic
--- PASS: TestAccFastlyServiceV1_s3logging_basic (35.53s)
=== RUN   TestAccFastlyServiceV1_s3logging_s3_env
--- PASS: TestAccFastlyServiceV1_s3logging_s3_env (15.95s)
=== RUN   TestAccFastlyServiceV1_s3logging_formatVersion
--- PASS: TestAccFastlyServiceV1_s3logging_formatVersion (16.60s)
=== RUN   TestAccFastlyServiceV1_sumologic
--- PASS: TestAccFastlyServiceV1_sumologic (15.80s)
=== RUN   TestAccFastlyServiceV1_updateDomain
--- PASS: TestAccFastlyServiceV1_updateDomain (36.83s)
=== RUN   TestAccFastlyServiceV1_updateBackend
--- PASS: TestAccFastlyServiceV1_updateBackend (37.11s)
=== RUN   TestAccFastlyServiceV1_basic
--- PASS: TestAccFastlyServiceV1_basic (13.72s)
=== RUN   TestAccFastlyServiceV1_disappears
--- PASS: TestAccFastlyServiceV1_disappears (7.30s)
=== RUN   TestAccFastlyServiceV1_defaultTTL
--- PASS: TestAccFastlyServiceV1_defaultTTL (54.30s)
=== RUN   TestAccFastlyServiceV1_VCL_basic
--- PASS: TestAccFastlyServiceV1_VCL_basic (34.76s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/fastly 512.060s

@catsby catsby merged commit 507917d into hashicorp:master Apr 18, 2017
@aerostitch
Copy link
Contributor Author

Thanks! :)

@aerostitch aerostitch deleted the default_ttl_issues_12910 branch April 18, 2017 18:39
@ghost
Copy link

ghost commented Apr 13, 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 13, 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.

provider/fastly: fastly_service_v1.default_ttl doesn't update when it's 0
3 participants