Skip to content
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

apiextension: simplify cases for XListType and XMapType in Structural #84986

Open
wants to merge 1 commit into
base: master
from

Conversation

@sttts
Copy link
Contributor

sttts commented Nov 8, 2019

We made the choice that x-kubernetes-list-type and map-type can be empty and atomic/granular. This is an ambiguous representation we better avoid in the internal structural data structure.

/kind cleanup

NONE
@sttts

This comment has been minimized.

Copy link
Contributor Author

sttts commented Nov 8, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

@sttts: GitHub didn't allow me to assign the following users: enxebre.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @liggitt @apelisse @enxebre

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 8, 2019

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.

@sttts sttts force-pushed the sttts:sttts-apiextension-simplify-list-type-map-type branch from 00ea1c4 to 3af3957 Nov 8, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sttts
To complete the pull request process, please assign liggitt
You can assign the PR to them by writing /assign @liggitt in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sttts sttts force-pushed the sttts:sttts-apiextension-simplify-list-type-map-type branch from 3af3957 to 7c2943c Nov 8, 2019
required("spec", "validation", "openAPIV3Schema", "properties[granular-map]", "not", "type"),
required("spec", "validation", "openAPIV3Schema", "properties[atomic-array]", "not", "type"),
},
},

This comment has been minimized.

Copy link
@sttts

sttts Nov 8, 2019

Author Contributor

@liggitt this is formally api, but only adds a test that we have not broken anything with the change.

@@ -104,29 +104,30 @@ type Extensions struct {
// x-kubernetes-list-type annotates a list to further describe its topology.
// This extension must only be used on lists and may have 3 possible values:
//
// 1) `atomic`: the list is treated as a single entity, like a scalar.
// 1) `` for atomic: the list is treated as a single entity, like a scalar.

This comment has been minimized.

Copy link
@sttts

sttts Nov 8, 2019

Author Contributor

we use empty to not break the elegant ForbiddenExtensions check that just compares to an empty Extensions struct.

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

I don't understand the "`` for atomic" change

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2019

Author Contributor

empty string if it is atomic (the default for arrays).


// x-kubernetes-map-type annotates an object to further describe its topology.
// This extension must only be used when type is object and may have 2 possible values:
//
// 1) `granular`:
// 1) `` for granular:

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

I don't understand this change

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2019

Author Contributor

empty string if it is granular (the default for maps). In other words: we only set this value for the non-standard cases.

@@ -104,29 +104,30 @@ type Extensions struct {
// x-kubernetes-list-type annotates a list to further describe its topology.
// This extension must only be used on lists and may have 3 possible values:
//
// 1) `atomic`: the list is treated as a single entity, like a scalar.
// 1) `` for atomic: the list is treated as a single entity, like a scalar.

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

I don't understand the "`` for atomic" change

if v.ForbiddenExtensions.XListType != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-list-type"), "must be undefined to be structural"))
if len(v.ForbiddenExtensions.XListType) != 0 {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-list-type"), "must be atomic to be structural"))

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

this error message doesn't make sense to me... if they set it to "atomic", won't this fail this validation?

if v.ForbiddenExtensions.XMapType != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-map-type"), "must be undefined to be structural"))
if len(v.ForbiddenExtensions.XMapType) != 0 {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-map-type"), "must be granular to be structural"))

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

same here, if they set it to "granular", won't it still fail validation with a message saying it must be "granular"?

}

if s.XListType != nil && *s.XListType != "atomic" {
ret.XListType = *s.XListType

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 12, 2019

Member

hmm... I don't like turning an explicit value into an implicit value. I'm also slightly surprised this doesn't break the round-trip test

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2019

Author Contributor

having two strings is a speciality of our CRD API. Structural intentionally abstracts that. But this one is slightly different to what we did before: before we were able to facefully represent the CRD schemas, but had another validation layer which restricted it (to structual schemas). This one is different because it normalizes.

Roundtrips probably do not break because we roundtrip valid Structural -> go-openapi/CRD schemas -> valid Structural.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.