New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix conversion of literal null JSON values #104969
Conversation
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go
Outdated
Show resolved
Hide resolved
{ | ||
name: "null values", | ||
in: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`, | ||
out: `{"default":{"test":null},"enum":[null],"example":{"test":null}}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to reject it if people are trying to make null
an enum option. That cannot work, it will break assumptions that null
means you are trying to clear the field. I can't tell if that is what is going on here or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a nullable value you want to constrain to a specific set of values or null must be able to include null as an enum value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A field being set to null is not a condition we can express, e.g. it can't round-trip. Null is an enum value the way that bald is a hair color. I would not expect to have to list null as a possible value in order to use field: null
to delete the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can/does round-trip in custom resources...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I realize what is going on here now, I agree raw JSON should work like this, given that we have it at all, which I'm not super happy about...
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go
Outdated
Show resolved
Hide resolved
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
5fe9823
to
89ae351
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/triage accepted |
…969-upstream-release-1.20 Automated cherry pick of #104969: Propagate conversion errors
…969-upstream-release-1.21 Automated cherry pick of #104969: Propagate conversion errors
…969-upstream-release-1.22 Automated cherry pick of #104969: Propagate conversion errors
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes conversion of literal null values in CRD schema definitions and fixes non-propagation of errors in conversion.
Does this PR introduce a user-facing change?
/cc @sttts @lavalamp