k8s: use NullableString instead of StringValue for preconditions #383
Conversation
The UpdatePods API uses nullable string values to represent annotations/labels preconditions. A null annotation value means that a pod should only be updated if the corresponding annotation is not currently set. grpc-gateway deserializes a null `StringValue` as a empty string instead of as a null pointer. This makes it impossible to use the current API to express that an annotation should not be set. This change introduces a new `NullableString` protobuf type to work around this issue.
| message NullableString { | ||
| oneof kind { | ||
| google.protobuf.NullValue null = 1; | ||
| string value = 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
not clear on why this diverges from the way that string value is written i.e.
message StringValue {
// The string value.
string value = 1;
}
also would prefer we make whatever solution it is global and copy all of the wrappers over to clutch/api/api/v1/wrappers.proto.
There was a problem hiding this comment.
I went for this approach in order to make it very explicit that the value is nullable and how to express a null value.
I don't however have a strong preference for the current version vs copying the wrapper types to Clutch — if you prefer the latter let me know and I'll update the PR.
There was a problem hiding this comment.
Yeah, though the wrapper types are poorly named (NullableString is a better name for sure), I don't want to introduce a new concept when the core proto ecosystem already has one. Simply copying the wrapper types into our own file to circumvent the pbjson flattening is to me the best fix and clearly communicates our intent.
in documenting that file we can refer to protobufjs/protobuf.js#677 and https://github.com/protocolbuffers/protobuf-go/blob/3f7a61f89bb6813f89d981d1870ed68da0b3c3f1/encoding/protojson/well_known_types.go#L35-L44
Description
The UpdatePods API uses nullable string values to represent
annotations/labels preconditions. A null annotation value means that a
pod should only be updated if the corresponding annotation is not
currently set.
grpc-gateway deserializes a null
StringValueas a empty string insteadof as a null pointer. This makes it impossible to use the current API to
express that an annotation should not be set.
This change introduces a new
NullableStringprotobuf type to workaround this issue.
Testing Performed
Unit tests + manual tests with a browser.