Make `cluster_agent_id` in Environment API nullable

[timofurrer]

This change set adds support to distinguish null and omitted values in the cluster_agent_id field of the edit environments API.

The added unit tests show how null and omitted ClusterAgentID values are differently marshaled.

The pulled in dependency is an Apache v2 license: http://www.apache.org/licenses/LICENSE-2.0

[timofurrer]

@svanharmelen any chance of getting a review on this? It's currently blocking some downstream work in the provider.

[svanharmelen]

The reason I didn't merge this yet is because (as you know by now) I'm a sucker for consistency. So introducing a change like this makes me wonder if we should update all the options structs to use these types so using the structs remains consistent for users of the package.

The downside is of course a very big breaking change (next to a little work to update all fields, but a few search and replaces should come a long way for that), so I'm still a little torn about this one...

[RicePatrick]

Should we maybe do it struct-by-struct? That way each struct is internally consistent even if it's not a giant change across the whole repo?

I'd be worried about the size of the PR to update every options struct, and whether that would introduce other odd bugs that we wouldn't be able to nicely catch in review...

[svanharmelen]

On one hand that makes sense, but on the other hand it means introducing a whole stream of backwards incompatible changes which doesn't sound like a good idea to me (would give a lot of update issues towards users of the package).

[RicePatrick]

Mmmm true. Better to get it out of the way all at once.

I think the trick is that some APIs (this one included) differentiate between a "0" value and "nil" when sending the edit - "nil" retains the current value while "0" removes the pre-existing value. The pointer system doesn't handle that well since omitempty will omit both nil and 0 from being sent. I seem to recall we've hit this in one other place at least (application settings, specifically with supported git protocols where nil has specific meaning).

I'm in support of updating all the pointers to nullable if we'd like to do that, but I'd also be OK with limiting this change to only APIs where nil vs the Go empty value has special meaning as well.

[timofurrer]

I think the trick is that some APIs (this one included) differentiate between a "0" value and "nil" when sending the edit - "nil" retains the current value while "0" removes the pre-existing value. The pointer system doesn't handle that well since omitempty will omit both nil and 0 from being sent. I seem to recall we've hit this in one other place at least (application settings, specifically with supported git protocols where nil has specific meaning).

That's the only reason why I've introduced this nullable type. It's that semantically the absence of a field is sometimes NOT equal to setting it to its zero-value. In this example, omitting the cluster_agent_id field means that the update endpoint should leave it untouched whereas a null value indicates that the cluster_agent_id field should be cleared and the association between an Environment and a Cluster Agent removed.

I'm actually wondering if the proposed omitzero tag would help here 🤔

I wouldn't recommend doing a big bang change and do this for all the fields nor the fields where it would make semantically sense.

I'm actually opting to dropping this change for now and handle it in a specific request func we pass to the update function - as we already do sometimes in the provider.

@svanharmelen @RicePatrick WDYT?

[timofurrer]

I've implemented custom marshalling the provider - although long-term I think it's valuable to add this "somehow" to go-gitlab, because it's something we want to support out-of-the-box.