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

🐛 Add support for empty maps or lists #863

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/crd/markers/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,9 @@ func (m Default) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
if err != nil {
return err
}
if schema.Type == "array" && string(marshalledDefault) == "{}" {
marshalledDefault = []byte("[]")
}
schema.Default = &apiext.JSON{Raw: marshalledDefault}
return nil
}
Expand Down
19 changes: 19 additions & 0 deletions pkg/crd/testdata/cronjob_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,18 @@ type CronJobSpec struct {
// +kubebuilder:example={{nested: {foo: "baz", bar: true}},{nested: {bar: false}}}
DefaultedObject []RootObject `json:"defaultedObject"`

// This tests that empty slice defaulting can be performed.
// +kubebuilder:default={}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is {} a valid default for slice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this needs to be fixed. CRD does not complain for deafult: [] though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is fixed in the current commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

It seems like {} is the correct syntax for an empty array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strange. But I guess fine given it's not JSON but our custom syntax (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's strange, but too late to change it now? Probably a reason for not using the obvious [] for arrays in marker syntax?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup seems definitely too late to change it now. Given that non-empty arrays worked already in the past :)

DefaultedEmptySlice []string `json:"defaultedEmptySlice"`

// This tests that an empty object defaulting can be performed on a map.
// +kubebuilder:default={}
DefaultedEmptyMap map[string]string `json:"defaultedEmptyMap"`

// This tests that an empty object defaulting can be performed on an object.
// +kubebuilder:default={}
DefaultedEmptyObject EmpiableObject `json:"defaultedEmptyObject"`

// This tests that pattern validator is properly applied.
// +kubebuilder:validation:Pattern=`^$|^((https):\/\/?)[^\s()<>]+(?:\([\w\d]+\)|([^[:punct:]\s]|\/?))$`
PatternObject string `json:"patternObject"`
Expand Down Expand Up @@ -297,6 +309,13 @@ type MinMaxObject struct {
Baz string `json:"baz,omitempty"`
}

type EmpiableObject struct {

// +kubebuilder:default=forty-two
Foo string `json:"foo,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a default to the field so that we can prove that the empty default on the parent triggers the inner field to be defaulted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with a modified version of CRD, where only these 2 new fields are present, this is how the applied CR looks like:

kind: CronJob
apiVersion: testdata.kubebuilder.io/v1
metadata:
  name: test
  namespace: default
spec: {}

results in

apiVersion: testdata.kubebuilder.io/v1
kind: CronJob
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"testdata.kubebuilder.io/v1","kind":"CronJob","metadata":{"annotations":{},"name":"test","namespace":"default"},"spec":{}}
  creationTimestamp: "2023-11-24T16:21:31Z"
  generation: 1
  name: test
  namespace: default
  resourceVersion: "23165"
  uid: d86f95e1-54be-4e04-b789-cb997bc3dd17
spec:
  defaultedEmptyObject:
    foo: forty-two
  defaultedEmptySlice: []

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoelSpeed Do you think all the comments are addressed? This is quite useful functionality, and it would be best to avoid manually patching resource, and rely on the annotation instead.

Bar string `json:"bar,omitempty"`
}

type unexportedStruct struct {
// This tests that exported fields are not skipped in the schema generation
Foo string `json:"foo"`
Expand Down
25 changes: 25 additions & 0 deletions pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,28 @@ spec:
- Forbid
- Replace
type: string
defaultedEmptyMap:
default: {}
description: This tests that an empty object defaulting can be performed on a map.
type: object
additionalProperties:
type: string
defaultedEmptyObject:
default: {}
description: This tests that an empty object defaulting can be performed on an object.
properties:
bar:
type: string
foo:
type: string
default: forty-two
type: object
defaultedEmptySlice:
default: []
description: This tests that empty slice defaulting can be performed.
items:
type: string
type: array
defaultedObject:
default:
- nested:
Expand Down Expand Up @@ -7434,6 +7456,9 @@ spec:
- baz
- binaryName
- canBeNull
- defaultedEmptyMap
- defaultedEmptyObject
- defaultedEmptySlice
- defaultedObject
- defaultedSlice
- defaultedString
Expand Down
4 changes: 3 additions & 1 deletion pkg/markers/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,15 +310,17 @@ func guessType(scanner *sc.Scanner, raw string, allowSlice bool) *Argument {
// We'll cross that bridge when we get there.

// look ahead till we can figure out if this is a map or a slice
hint = peekNoSpace(subScanner)
firstElemType := guessType(subScanner, subRaw, false)
if firstElemType.Type == StringType {
// might be a map or slice, parse the string and check for colon
// (blech, basically arbitrary look-ahead due to raw strings).
var keyVal string // just ignore this
(&Argument{Type: StringType}).parseString(subScanner, raw, reflect.Indirect(reflect.ValueOf(&keyVal)))

if subScanner.Scan() == ':' {
if token := subScanner.Scan(); token == ':' || hint == '}' {
// it's got a string followed by a colon -- it's a map
// or an empty map in case of {}
return &Argument{
Type: MapType,
ItemType: &Argument{Type: AnyType},
Expand Down
1 change: 1 addition & 0 deletions pkg/markers/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ var _ = Describe("Parsing", func() {
It("should support delimitted slices of delimitted slices", argParseTestCase{arg: sliceOSlice, raw: "{{1,1},{2,3},{5,8}}", output: sliceOSliceOut}.Run)

It("should support maps", argParseTestCase{arg: Argument{Type: MapType, ItemType: &Argument{Type: StringType}}, raw: "{formal: hello, `informal`: `hi!`}", output: map[string]string{"formal": "hello", "informal": "hi!"}}.Run)
It("should work with empty maps (which are equal to empty lists in the output)", argParseTestCase{arg: Argument{Type: MapType, ItemType: &Argument{Type: StringType}}, raw: "{}", output: map[string]string{}}.Run)

Context("with any value", func() {
anyArg := Argument{Type: AnyType}
Expand Down