cmd/cue: detect kubernetes semantics for get go
#4191
Conversation
mvdan
left a comment
mvdan
left a comment
There was a problem hiding this comment.
SGTM overall, but I think this implementation isn't quite complete - it should support the "optional" and "nullable" field comment attributes that the k8s openapi tool supports. Note that we already support +optional (and I'm not sure if that's already compatible with what k8s does), but we don't support +nullable at all.
|
Also, could you split this in two commits as follows:
|
fionera
force-pushed
the
kubernetes-semantics
branch
2 times, most recently
from
da2c98e to
289db25
Compare
mvdan
left a comment
mvdan
left a comment
There was a problem hiding this comment.
LGTM - just a few last nits and I'm happy to import into Gerrit and merge
We are currently generating "wrong" schemas from K8s source when comparing against their OpenAPI spec. Change-Id: Ieb30f98ba2bdc921ab48c81aedbdc89c685b4bd9 Signed-off-by: Tim Windelschmidt <[email protected]>
Kubernetes types with either +optional or omitempty are expected to not have null disjunctions. Fields with +nullable are required and have a valid null value. By checking for the k8s openapi annotations we can switch to a kubernetes semantic mode and generate the schema correctly. Change-Id: I747de2ddda57f8bc079afe9180a8179fd5b2c778 Signed-off-by: Tim Windelschmidt <[email protected]>
fionera
force-pushed
the
kubernetes-semantics
branch
from
289db25 to
d7f763b
Compare
mvdan
left a comment
mvdan
left a comment
There was a problem hiding this comment.
Thanks! LGTM with some last nits.
When you're ready, could you please send these for a final review and test/merge on Gerrit? The way we import PRs to Gerrit doesn't really support multi-commit PRs unless we squash them, and I don't want to squash here.
| // +optional | ||
| OptionalCommentPointer *string `json:"optionalCommentPointer"` | ||
| // +nullable | ||
| NullableCommentPointer *string `json:"nullableCommentPointer"` |
There was a problem hiding this comment.
I just realised - we need to include a field which is both optional and nullable. it should generate as...
foo?: null | string
There was a problem hiding this comment.
Turns out that should be the default behavior:
Optional fields have the following properties:
They have the +optional comment tag in Go.
They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag) or have a built-in nil value (e.g. maps and slices).
They are marked with the omitempty json struct tag in the Go definition.
The API server should allow POSTing and PUTing a resource with this field unset.
| return regular | ||
| case "+optional": | ||
| return optional | ||
| } |
There was a problem hiding this comment.
I think that the way this loops over the doc lines, and returns at the first hit, will not work well when a field is marked as BOTH optional and nullable. You might need to make fieldKind a bit set, so that we can track that a field was marked as either or both optional/nullable?
If this is not common and you want to leave this tweak for a future PR, that's fine, but then we'd need a TODO.
There was a problem hiding this comment.
I actually changed the whole PR 😅 I am gonna close this one and push it to gerrit as its probably a better review platform
2 participants
Kubernetes types with either +optional or omitempty, are expected to not have a null disjunction. By checking for their annotations we can switch to a kubernetes semantic mode and not generate them.