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

Cloudflare LoadBalancer support for Pool Latitude and Longitude #1093

Merged

Conversation

tpolich
Copy link

@tpolich tpolich commented Jun 3, 2021

Added support for Latitude/Longitude for load balancer pools.

Removed duplicated float validation function replaced with standard version from validation package.

go.mod Outdated
@@ -3,7 +3,7 @@ module github.com/cloudflare/terraform-provider-cloudflare
go 1.15

require (
github.com/cloudflare/cloudflare-go v0.17.0
github.com/cloudflare/cloudflare-go v0.17.1-0.20210602222010-1740533cf449
Copy link
Member

Choose a reason for hiding this comment

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

are you able to remove this from the PR? we generally bump dependencies outside of the functionality and then just merge the feature once it is ready. this does mean we're pending the release of cloudflare-go but that is generally 👌

Copy link
Author

Choose a reason for hiding this comment

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

I don't think I can that hash is the first commit containing the Latitude and Longitude fields on cloudflare.LoadBalancerPool they don't exist in the last released version.

Copy link
Member

Choose a reason for hiding this comment

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

yep, that's expected. we'll have an error on the build here in GitHub but once we cut a new release of cloudflare-go, this will get the update and won't have to deal with conflicts as it will change underneath it. if we leave this here and in other PRs, all of them will have to resolve go.mod and go.sum conflicts upon update which are no fun for anyone.

when i pull the tests locally, i can shim in the latest master version too using go's replace in the go.mod file to roughly check it out.

Copy link
Author

Choose a reason for hiding this comment

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

makes sense I've removed the mod changes from this PR

@jacobbednarz
Copy link
Member

we'll also want to update https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/website/docs/r/load_balancer.html.markdown to add the new values and descriptions to the website documentation.

@tpolich tpolich force-pushed the load_balancer_proximity_support_v2 branch from 393f56f to 83a75e3 Compare June 3, 2021 21:52
@tpolich
Copy link
Author

tpolich commented Jun 3, 2021

we'll also want to update https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/website/docs/r/load_balancer.html.markdown to add the new values and descriptions to the website documentation.

I always forget about that. I've added latitude and longitude to the pool documentation.

@tpolich tpolich force-pushed the load_balancer_proximity_support_v2 branch from 83a75e3 to 2875b73 Compare June 3, 2021 23:44
Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

lgtm, just waiting on cloudflare-go's release to get this merged

@jacobbednarz jacobbednarz added the workflow/pending-upstream-library Indicates an issue or PR requires changes from an upstream library. label Jun 15, 2021
@jacobbednarz jacobbednarz force-pushed the load_balancer_proximity_support_v2 branch from 2875b73 to 6cf400b Compare June 23, 2021 03:35
@jacobbednarz jacobbednarz removed the workflow/pending-upstream-library Indicates an issue or PR requires changes from an upstream library. label Jun 23, 2021
@jacobbednarz
Copy link
Member

Rebased in latest master to get the latest cloudflare-go changes.

@jacobbednarz
Copy link
Member

jacobbednarz commented Jun 23, 2021

looks like we've got a type issue here with the float64 vs *float32

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareLoadBalancerPool" -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareLoadBalancerPool_Import
=== PAUSE TestAccCloudflareLoadBalancerPool_Import
=== RUN   TestAccCloudflareLoadBalancerPool_Basic
=== PAUSE TestAccCloudflareLoadBalancerPool_Basic
=== RUN   TestAccCloudflareLoadBalancerPool_FullySpecified
=== PAUSE TestAccCloudflareLoadBalancerPool_FullySpecified
=== RUN   TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== PAUSE TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== CONT  TestAccCloudflareLoadBalancerPool_Import
--- PASS: TestAccCloudflareLoadBalancerPool_Import (4.82s)
=== CONT  TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
--- PASS: TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy (6.14s)
=== CONT  TestAccCloudflareLoadBalancerPool_FullySpecified
panic: interface conversion: interface {} is float64, not *float32

goroutine 1663 [running]:
github.com/cloudflare/terraform-provider-cloudflare/cloudflare.resourceCloudflareLoadBalancerPoolCreate(0xc0005a53b0, 0x2289a60, 0xc000201080, 0x2, 0x2d8d1c0)
	/Users/jacob/go/src/github.com/cloudflare/terraform-provider-cloudflare/cloudflare/resource_cloudflare_load_balancer_pool.go:169 +0x953
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Resource).Apply(0xc00030d180, 0xc000d09900, 0xc001019420, 0x2289a60, 0xc000201080, 0x20b5801, 0xc0010942b8, 0xc0010157d0)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.17.2/helper/schema/resource.go:320 +0x375
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Provider).Apply(0xc0006b6480, 0xc00107da38, 0xc000d09900, 0xc001019420, 0xc001017048, 0xc001007ab0, 0x20b87a0)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.17.2/helper/schema/provider.go:294 +0x99
github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).ApplyResourceChange(0xc00018a378, 0x2518f60, 0xc001008e70, 0xc0005a4ee0, 0xc00018a378, 0xc001008e70, 0xc00011aba0)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.17.2/internal/helper/plugin/grpc_provider.go:895 +0x8ab
github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5._Provider_ApplyResourceChange_Handler(0x2238fc0, 0xc00018a378, 0x2518f60, 0xc001008e70, 0xc000afed80, 0x0, 0x2518f60, 0xc001008e70, 0xc001010400, 0x3c6)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.17.2/internal/tfplugin5/tfplugin5.pb.go:3305 +0x214
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0002c0c40, 0x2527a60, 0xc000c2a180, 0xc00073a900, 0xc00090c7b0, 0x2d47980, 0x0, 0x0, 0x0)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/google.golang.org/grpc@v1.32.0/server.go:1194 +0x522
google.golang.org/grpc.(*Server).handleStream(0xc0002c0c40, 0x2527a60, 0xc000c2a180, 0xc00073a900, 0x0)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/google.golang.org/grpc@v1.32.0/server.go:1517 +0xd05
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00089e050, 0xc0002c0c40, 0x2527a60, 0xc000c2a180, 0xc00073a900)
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/google.golang.org/grpc@v1.32.0/server.go:859 +0xa5
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/Users/jacob/.asdf/installs/golang/1.15.6/packages/pkg/mod/google.golang.org/grpc@v1.32.0/server.go:857 +0x1fd
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	11.639s
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]
FAIL
make: *** [testacc] Error 1

@jacobbednarz
Copy link
Member

Ah, I remembered why. Terraform schema TypeFloat is float64, not float32. You'll need to convert it in your schema definition for this to work.

@tpolich tpolich force-pushed the load_balancer_proximity_support_v2 branch from 6cf400b to 76a2583 Compare June 25, 2021 22:32
@jacobbednarz
Copy link
Member

We're up and running now but it looks like the float is extremely precise 😛

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareLoadBalancerPool" -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareLoadBalancerPool_Import
=== PAUSE TestAccCloudflareLoadBalancerPool_Import
=== RUN   TestAccCloudflareLoadBalancerPool_Basic
=== PAUSE TestAccCloudflareLoadBalancerPool_Basic
=== RUN   TestAccCloudflareLoadBalancerPool_FullySpecified
=== PAUSE TestAccCloudflareLoadBalancerPool_FullySpecified
=== RUN   TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== PAUSE TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== CONT  TestAccCloudflareLoadBalancerPool_Import
--- PASS: TestAccCloudflareLoadBalancerPool_Import (3.54s)
=== CONT  TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
--- PASS: TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy (6.32s)
=== CONT  TestAccCloudflareLoadBalancerPool_FullySpecified
    testing.go:705: Step 0 error: Check failed: Check 6/8 error: cloudflare_load_balancer_pool.ducrcjbrmx: Attribute 'latitude' expected "12.3", got "12.300000190734863"
--- FAIL: TestAccCloudflareLoadBalancerPool_FullySpecified (2.82s)
=== CONT  TestAccCloudflareLoadBalancerPool_Basic
--- PASS: TestAccCloudflareLoadBalancerPool_Basic (3.22s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	16.368s
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]
FAIL
make: *** [testacc] Error 1

Do we want to trim this to only store a single degree of accuracy instead of 15?

Removed duplicated float validation function replaced with standard
version from validation package.
@tpolich tpolich force-pushed the load_balancer_proximity_support_v2 branch from 76a2583 to 82b9c9c Compare July 16, 2021 23:07
@jacobbednarz jacobbednarz force-pushed the load_balancer_proximity_support_v2 branch from b475b6f to 6e45ba3 Compare July 18, 2021 23:55
@jacobbednarz
Copy link
Member

looking good here too!

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareLoadBalancerPool_" -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareLoadBalancerPool_Import
=== PAUSE TestAccCloudflareLoadBalancerPool_Import
=== RUN   TestAccCloudflareLoadBalancerPool_Basic
=== PAUSE TestAccCloudflareLoadBalancerPool_Basic
=== RUN   TestAccCloudflareLoadBalancerPool_FullySpecified
=== PAUSE TestAccCloudflareLoadBalancerPool_FullySpecified
=== RUN   TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== PAUSE TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
=== CONT  TestAccCloudflareLoadBalancerPool_Import
--- PASS: TestAccCloudflareLoadBalancerPool_Import (3.19s)
=== CONT  TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy
--- PASS: TestAccCloudflareLoadBalancerPool_CreateAfterManualDestroy (4.83s)
=== CONT  TestAccCloudflareLoadBalancerPool_FullySpecified
--- PASS: TestAccCloudflareLoadBalancerPool_FullySpecified (2.79s)
=== CONT  TestAccCloudflareLoadBalancerPool_Basic
--- PASS: TestAccCloudflareLoadBalancerPool_Basic (2.54s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	14.058s
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

@jacobbednarz jacobbednarz merged commit cd759bf into cloudflare:master Jul 18, 2021
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.

2 participants