Consolidate the OidcAuthentication properties on FeedResource into a single property#341
Conversation
…server side binding
… creating one in mapping methods with incomplete data
IsaacCalligeros95
changed the title
wlthomson
left a comment
wlthomson
left a comment
There was a problem hiding this comment.
Not an expert in this area, but this looks OK to me.
I'm not sure about the unmarshelling without some sort of "type" property as pointed out, but without that I can't see a better alternative.
Couple of minors, but otherwise good to go ahead.
|
|
||
| // Discriminate based on properties. This is hacky, but we would have to add a "Type" property to the API | ||
| // To handle this properly | ||
| if _, hasClientId := raw["ClientId"]; hasClientId { |
There was a problem hiding this comment.
Do you also need to check that the value isn't empty?
There was a problem hiding this comment.
No, the ClientId can be empty, but the field is still returned on the OidcAuthentication object and used as the discriminator to get the type
There was a problem hiding this comment.
Oh, didn't realise ClientId was optional. In either case, just flagging this up as a small regression risk as we need to be careful that other shapes aren't changed at any point to include null/undefined/empty properties which could trigger the wrong conditional (seems unlikely, but possible). I'm not sure what to suggest for optional fields, though.
3 participants
When updating ACR, GCR and ECS feeds the OidcAuthentication property is not being bound on the server side modify calls. This happens because the FeedResource properties
AzureContainerRegistryOidcAuthentication,ElasticContainerRegistryOidcAuthenticationandGoogleContainerRegistryOidcAuthenticationdo not match the server side values. This change consolidates these into a single property OidcAuthentication which has a type property to discriminate on. The JSON unmarshalling here isn't pretty but works, it determines the OidcAuthentication type based on the available properties, in this case the properties are always available, and we're just checking it's there the value itself is not required.Related to - https://github.com/OctopusDeploy/terraform-provider-octopusdeploy/pull/40/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6