add enabled to LoadBalancer struct #273
Conversation
There was a problem hiding this comment.
Thanks for picking this one up @SteveGoldthorpe-Work! Could you please update the tests that are currently failing? They are related to your PR changes.
I'll raise a ticket with Cloudflare to get the API team to update the docs.
|
Raised via 1639684 with Cloudflare Support. |
As I feared, you can't use omitempty with a bool type properly as false is the same as 0/null/empty. Implemented with a pointer that can determine if it's not set. Seems to work with my provider code (which defaults the value to true). |
| @@ -66,6 +66,7 @@ type LoadBalancer struct { | |||
| RegionPools map[string][]string `json:"region_pools"` | |||
| PopPools map[string][]string `json:"pop_pools"` | |||
| Proxied bool `json:"proxied"` | |||
| Enabled *bool `json:"enabled,omitempty"` | |||
There was a problem hiding this comment.
What was the thinking here as to using a pointer? We always have a value provided in the API responses I've seen so this should be a bool (not *bool).
If it's in regards to making the tests green, you just needed to add the enabled values to the test and it should be golden.
There was a problem hiding this comment.
No, it about not omitting false values, which I was worried about when I created the original PR (and why I did not use omitempty and made it fail the tests).
There's precedent in zone.go:
commit ea9272e
Author: James O'Gorman james@netinertia.co.uk
Date: Sun Feb 19 20:42:38 2017 +0000
fix(zone): fix the ability to un-pause zones
Due to using `omitempty` to not send unset values to the API, this also
meant that `false` values for `paused` would also not be sent.
Using a pointer instead fixes this issue.
Closes #39
There was a problem hiding this comment.
I'm not entirely sure if that's the same issue here as the "enabled" value is always present and uses a PUT endpoint for updates (read full replacement) whereas the zone update endpoint is PATCH (partial updates).
There was a problem hiding this comment.
When using a simple bool in the struct, if I don't use omitempty, then I introduce "enabled: false" to every LoadBalancer struct we don't explicitly set the enabled field. This is why the original tests failed and is not desirable. If anything we'd probably prefer it to default to true. If I do use omitempty with a simple bool, then false values won't be passed onto the API as they will be omitted, so we can never disable a load balancer.
I think it has to be a pointer to allow for the values of unset, true and false.
I'm not a native go programmer so someone more familiar may wish to review this.
There was a problem hiding this comment.
Gotcha, it's the default values causing pain here. There are a couple of ways to Do This Better but I think we'd look to apply that across the board in one fell swoop instead of dribs and drabs.
|
Anything stopping this being merged? it's delaying https://github.com/terraform-providers/terraform-provider-cloudflare/issues/208 |
|
@SteveGoldthorpe-Work one of the primary maintainers of this library is currently on paternity leave. I'll ping the Cloudflare folks to see if we can get some other eyes here as a few PRs are blocking terraform provider changes now. |
|
I'm back! Sorry for the delay and thank you @SteveGoldthorpe-Work for patience. |
Description
Add an Enabled bool field so that load balancers can be enabled or disabled.
Issue #272
Has your change been tested?
Briefly with my own custom terraform cloudflare provider.
I'm not sure if adding an extra field will cause any issues - it doesn't in my limited testing.