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

How to enum poorly documented #1071

Closed
erictune opened this issue Oct 4, 2019 · 12 comments
Closed

How to enum poorly documented #1071

erictune opened this issue Oct 4, 2019 · 12 comments
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@erictune
Copy link

erictune commented Oct 4, 2019

There are quite a few ways a user might try to make an enum. Only one or two ways are "right".

Suggested Fix:

  • Specifically have a section in the KubeBuilder book called "Enums" and show a generic example.
  • Explain that the validation and how you chose to define types in Go are not related. (This is not obvious because some aspect of go syntax do generate OpenAPI, like scalar types.)

All the ways

The Right Way

The documentation example for CronJob does an enum like this:

// +kubebuilder:validation:Enum=A,B,C
type MyEnum string
const (
    MyEnumA MyEnum = "A"
    MyEnumB MyEnum = "B"
    MyEnumB MyEnum = "C"
)
type MyObjectSpec {
  // 
   Mode MyEnum
}

This works right, in that the OpenAPI enum= validation is generated in the CRD.
However, a reasonable user might also try to do many other ways, only some of which work right.

Alternative One

kubebuilder comment on the field in the Object.
Works right

type MyEnum string
const (
    MyEnumA MyEnum = "A"
    MyEnumB MyEnum = "B"
    MyEnumB MyEnum = "C"
)
type MyObjectSpec {
  // 
   // +kubebuilder:validation:Enum=A,B,C
   Mode MyEnum
}

Alternative Two

kubebuilder comment on the field in the Object and on the EnumType.
Does not work. They seem to cancel out

// +kubebuilder:validation:Enum=A,B,C
type MyEnum string
const (
    MyEnumA MyEnum = "A"
    MyEnumB MyEnum = "B"
    MyEnumB MyEnum = "C"
)
type MyObjectSpec {
  // 
   // +kubebuilder:validation:Enum=A,B,C
   Mode MyEnum
}

Alternative Three

No kubebuilder comment.
does not work
User might assume that when you use the Go idiom for an enum that the OpenAPI is inferred. It isn't.

type MyEnum string
const (
    MyEnumA MyEnum = "A"
    MyEnumB MyEnum = "B"
    MyEnumB MyEnum = "C"
)
type MyObjectSpec {
   Mode MyEnum
}

Alternative Four

Kubebuilder comment and no Go enum idiom
works but no static checking in go

type MyObjectSpec {
  // 
   // +kubebuilder:validation:Enum=A,B,C
   Mode string
}

/kind documentation

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. labels Oct 4, 2019
@erictune
Copy link
Author

erictune commented Oct 4, 2019

/remove-kind bug

@k8s-ci-robot k8s-ci-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 2019
@DirectXMan12
Copy link
Contributor

Does not work. They seem to cancel out

Err... that's a bug

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 2019
@DirectXMan12
Copy link
Contributor

/good-first-issue

on the docs

help wanted on the cancelling out bug. I'm guessing that our allof resolution logic has a bug. Start by adding a unit test (It("should preserve validation when both the field and the type have the same validation")) and go from there.

@k8s-ci-robot
Copy link
Contributor

@DirectXMan12:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

on the docs

help wanted on the cancelling out bug. I'm guessing that our allof resolution logic has a bug. Start by adding a unit test (It("should preserve validation when both the field and the type have the same validation")) and go from there.

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.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Oct 4, 2019
@camilamacedo86
Copy link
Member

/assign @camilamacedo86

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2020
@djzager
Copy link

djzager commented Feb 12, 2020

/assign @djzager

@djzager
Copy link

djzager commented Feb 12, 2020

Looking into the doc portion of this issue it appears that this is already laid out in Generating CRDs. Specifically, the section concerning validation shows the correct way to do this (shortened for simplicity):

type ToySpec struct {
...
	Alias   Alias   `json:"alias,omitempty"`
	Rank    Rank    `json:"rank"`
}

// +kubebuilder:validation:Enum=Lion;Wolf;Dragon
type Alias string

@djzager
Copy link

djzager commented Feb 12, 2020

help wanted on the cancelling out bug. I'm guessing that our allof resolution logic has a bug. Start by adding a unit test (It("should preserve validation when both the field and the type have the same validation")) and go from there.

Maybe this hsould be an issue against https://github.com/kubernetes-sigs/controller-tools ? I don't see anything in kubebuilder related to that, but searching for allof in controller-tools leads me to believe this should be addressed there.

@DirectXMan12 what do you think?

@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 13, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

6 participants