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 +default markers #938

Merged
merged 1 commit into from
May 15, 2024

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented May 3, 2024

This adds annotation support for +default markers for specifying default values in JSON format, as is done in core Kubernetes types and described in https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1929-built-in-default#summary

If both +default and +kubebuilder:default are specified, the kubebuilder-specific annotation wins.

Fixes #939

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 3, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 3, 2024
@liggitt liggitt changed the title Add support for +default markers ✨ Add support for +default markers May 3, 2024
@liggitt liggitt changed the title ✨ Add support for +default markers ✨ Add support for +default markers May 3, 2024
@liggitt liggitt changed the title ✨ Add support for +default markers ✨ Add support for +default markers May 3, 2024
pkg/markers/parse.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Contributor Author

liggitt commented May 3, 2024

/hold

forgot about the ref(type) support in the KEP... that's going to be a pain to reimplement :-/

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2024
@liggitt
Copy link
Contributor Author

liggitt commented May 6, 2024

cc @alexzielenski for thoughts on the best way to add support for +default=ref(...) to kubebuilder

@alexzielenski
Copy link

cc @alexzielenski for thoughts on the best way to add support for +default=ref(...) to kubebuilder

In our code generators we use ParseSymbolReference to parse the ref string into an symbol name and package path. But our output is not the final YAML schema like it is for kubebuilder - it is Go code to express a spec.Schema.

To add ref support to this I don't see a way except a large refactor to generate code that expresses a spec.Schema like kube-openapi, or if there is a way to statically evaluate a constant using Go tools which I have not seen.

@liggitt
Copy link
Contributor Author

liggitt commented May 7, 2024

To add ref support to this I don't see a way except a large refactor to generate code that expresses a spec.Schema like kube-openapi, or if there is a way to statically evaluate a constant using Go tools which I have not seen.

yeah... it's really thorny... I might start by limiting support here to inlined json-formatted values. We don't actually use ref() in any in-tree objects... we might not want to as long as it's difficult for CRD generators to make use of them

@liggitt
Copy link
Contributor Author

liggitt commented May 7, 2024

Reworked this to delegate specialized marker parsing to the value holder, structured to ignore +default=ref(...) for now, and added test coverage for that.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2024
@alexzielenski
Copy link

I might start by limiting support here to inlined json-formatted values.

Sounds reasonable. I've looked around a little, and closest thing I found to static evaluation of constants from symbol path is go doc:

go doc k8s.io/api/core/v1.TerminationMessageReadFile
package v1 // import "k8s.io/kubernetes/staging/src/k8s.io/api/core/v1"

const (
        // TerminationMessageReadFile is the default behavior and will set the container status message to
        // the contents of the container's terminationMessagePath when the container exits.
        TerminationMessageReadFile TerminationMessagePolicy = "File"
        // TerminationMessageFallbackToLogsOnError will read the most recent contents of the container logs
        // for the container status message when the container exits with an error and the
        // terminationMessagePath has no contents.
        TerminationMessageFallbackToLogsOnError TerminationMessagePolicy = "FallbackToLogsOnError"
)

Unfortunately it is not machine readable, not isolated to what was asked for, and returns go source so if the constant was an expression like 24 * time.Hour then we'd not be able to handle it simply by parsing this.

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a few nits, otherwise lgtm

pkg/crd/testdata/cronjob_types.go Show resolved Hide resolved
pkg/crd/testdata/cronjob_types.go Show resolved Hide resolved
@sbueringer
Copy link
Member

Thx!

/lgtm
/assign @alvaroaleman

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3a1e540dedac1ba2aae86a0a41d631d8ff0255d4

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit 85686cb into kubernetes-sigs:master May 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubebuilder doesn't respect +default
5 participants