Handle required nullable types. #329
Conversation
The omicron api includes fields that are nullable (i.e. may be explicitly set to null) but required (i.e. they can't be omitted). To handle these types in the sdk, we treat them as pointers, and don't set `omitempty` in the struct tags. This patch also updates omicron to latest to pull in newly added or updated fields.
oxide/types.go
Outdated
| Memory ByteCount `json:"memory,omitempty" yaml:"memory,omitempty"` | ||
| // Ncpus is the number of CPUs to assign to this instance. | ||
| // Ncpus is the number of vCPUs to be allocated to the instance | ||
| Ncpus InstanceCpuCount `json:"ncpus,omitempty" yaml:"ncpus,omitempty"` |
There was a problem hiding this comment.
I know this can't be a big problem since it was already like this and these were already required, but why are these omitempty if they can't actually be omitted? Is it that zero isn't a valid value for these fields anyway?
There was a problem hiding this comment.
Oh yeah, I agree. I updated the logic to drop omitempty for these fields, and verified that the terraform provider acceptance tests still pass after making the change. I don't consider this to be a breaking change, so I'm fine with the large diff in the generated code.
jmcarp
force-pushed
the
jmcarp/handle-required-nullable
branch
from
2950f7b to
5799db9
Compare
lgfa29
left a comment
lgfa29
left a comment
There was a problem hiding this comment.
LGTM. I ran a quick test on r16 just in case and everything seems to still work 👍
sudomateo
left a comment
sudomateo
left a comment
There was a problem hiding this comment.
Sorry for the delay on this. Had a slow start today. I'm glad others noted the omit* issues on types that should no longer be omitted. I noticed that earlier but didn't get a chance to write it down.
I left one comment but otherwise this looks good.
It doesn't really make sense to omit empty or zero values for fields that are required by the api. This patch updates the generator to omit the `omitempty` struct tag for these fields. h/t @david-crespo
jmcarp
force-pushed
the
jmcarp/handle-required-nullable
branch
from
5799db9 to
953698f
Compare
The omicron api includes fields that are nullable (i.e. may be explicitly set to null) but required (i.e. they can't be omitted). To handle these types in the sdk, we treat them as pointers, and don't set
omitemptyin the struct tags.This patch also updates omicron to latest to pull in newly added or updated fields.