Feature: Added support for OpenAPI validations marker comments #435

Schnides123 · 2023-10-16T20:10:36Z

This PR adds the ability to annotate API fields with basic OpenAPI schema validations via marker comments, as part of the KEP-4153: Declarative Validation feature.

/assign @alexzielenski

linux-foundation-easycla · 2023-10-16T20:10:44Z

The committers listed above are authorized under a signed CLA.

✅ login: Schnides123 / name: Alex Schneider (a06b517)

k8s-ci-robot · 2023-10-16T20:10:47Z

Welcome @Schnides123!

It looks like this is your first PR to kubernetes/kube-openapi 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-openapi has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

alexzielenski · 2023-10-16T20:15:50Z

/ok-to-test

alexzielenski

Really like the approach. needs a lot more testing, though.

When we reach a more mature PR state we should open a k/k PR with these changes to show the diff with the generated OpenAPI

We can merge this next release after code thaw

pkg/generators/markers_test.go

pkg/generators/openapi.go

pkg/generators/markers.go

pkg/generators/openapi.go

k8s-ci-robot · 2023-10-16T20:28:51Z

@alexzielenski: GitHub didn't allow me to request PR reviews from the following users: WDYT, is, preferable.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

An alternative to using these helper functions is to do as you did in the test by abusing the array literal notation:
&[]float64{20.0}[0]
/cc @apelisse WDYT is preferable

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.

pkg/generators/markers.go

apelisse · 2023-10-18T22:57:52Z

I would still add an intergration test. Create a new directory for value validation in https://github.com/kubernetes/kube-openapi/tree/master/test/integration/testdata, add some types in there with various validation markers, carefully follow the README including the notes at the bottom. Thanks!

pkg/generators/markers.go

test/integration/testdata/valuevalidation/alpha.go

Schnides123 · 2023-12-08T00:00:51Z

I have been told to add the prefix back in, so the prefix is now back in

pkg/generators/markers.go

pkg/generators/markers_test.go

pkg/generators/openapi.go

test/integration/testdata/valuevalidation/alpha.go

alexzielenski

Looking really good. Few more tests before I think it is ready to go

pkg/generators/markers.go

pkg/generators/openapi.go

alexzielenski · 2023-12-09T00:27:43Z

pkg/generators/openapi.go

-				"Format:$.type|raw${}.OpenAPISchemaFormat(),\n"+
-				"},\n"+
+				"Format:$.type|raw${}.OpenAPISchemaFormat(),\n", args)
+			g.generateValueValidations(&overrides.SchemaProps)


I see the following new calls to generateValueValidations

Custom V2 Type/Format and V3 Definition Functions

Custom V2 Type/Format and V3 oneOf types

Custom V2 Type/Format (and No V3 Definition or OneOf)

V3 OneOf Types

Regular Behavior (no custom types or override functions implemented)

The checked boxes are those that an integration exists already. This is function is pretty complicated. I wonder if we can simplify this function in a future PR. In the meantime, I'd like to see the rest of the cases covered in the integration tests to make sure the output compiles in those cases.

alexzielenski · 2023-12-09T00:33:35Z

pkg/generators/markers.go

+		newKey := strings.TrimPrefix(key, prefix)
+
+		// Skip ref markers
+		if len(value) == 1 && regexp.MustCompile(`^(?:\s*ref\s{0,1}\().*\)\s*`).MatchString(value[0]) {


Is this possible try "k8s.io/gengo/examples/defaulter-gen/generators".ParseSymbolReference against the value to see if it is a ref instead? To avoid duplciating this logic

pkg/generators/markers.go

alexzielenski · 2023-12-14T18:45:47Z

pkg/generators/markers.go

@@ -59,6 +139,11 @@ func ParseCommentTags(comments []string, prefix string) (CommentTags, error) {
 		return CommentTags{}, fmt.Errorf("failed to unmarshal marker comments: %w", err)
 	}

+	// Validate the parsed comment tags
+	if err := commentTags.Validate(*t); err != nil {


I think id prefer to not validate within the parse function. Id expect the caller to call validate on the return value rather than to make this function take a type

Are you sure? I feel like there really isn't a use-case for generating comment tags that wouldn't pass validation. Splitting it out might be convenient for plumbing the type out of the ParseCommentTags arguments, but I don't foresee us having any situations where we'd want to create a commentTags instance and then not validate it immediately after.

pkg/generators/markers_test.go

pkg/generators/markers.go

alexzielenski · 2024-01-03T01:48:34Z

/label tide/merge-method-squash
/lgtm

alexzielenski

/approve

Looks really good! XXL PR but is primarily test code. Great starting point for the first alpha. Thanks for the extensive testing!

I pinned k/k against this branch and tested there are no dep changes (other than kube-openapi) and that running the code generator (without using the markers) does not result in changes

alexzielenski · 2024-01-03T03:40:47Z

/hold

can you squash (tide concats the descriptions which is not useful)

edit: i squashed for you

alexzielenski · 2024-01-03T04:33:56Z

/ok-to-test

shelved multiline marker comments for another PR

alexzielenski · 2024-01-03T05:09:03Z

/hold cancel
/lgtm

k8s-ci-robot · 2024-01-03T05:09:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, Schnides123

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:

~~OWNERS~~ [alexzielenski]

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 assigned alexzielenski Oct 16, 2023

k8s-ci-robot requested review from apelisse and Jefftree October 16, 2023 20:10

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 16, 2023

alexzielenski suggested changes Oct 16, 2023

View reviewed changes

k8s-ci-robot requested a review from thockin October 16, 2023 20:28

alexzielenski reviewed Oct 16, 2023

View reviewed changes

pkg/generators/markers.go Show resolved Hide resolved

apelisse reviewed Oct 16, 2023

View reviewed changes

pkg/generators/markers.go Outdated Show resolved Hide resolved

apelisse reviewed Oct 16, 2023

View reviewed changes

pkg/generators/markers.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2023

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 18, 2023

Jefftree reviewed Oct 23, 2023

View reviewed changes

pkg/generators/markers.go Show resolved Hide resolved

apelisse reviewed Oct 24, 2023

View reviewed changes

test/integration/testdata/valuevalidation/alpha.go Show resolved Hide resolved

alexzielenski suggested changes Dec 8, 2023

View reviewed changes

pkg/generators/markers.go Outdated Show resolved Hide resolved

pkg/generators/markers.go Outdated Show resolved Hide resolved

pkg/generators/markers_test.go Outdated Show resolved Hide resolved

pkg/generators/openapi.go Outdated Show resolved Hide resolved

alexzielenski reviewed Dec 8, 2023

View reviewed changes

test/integration/testdata/valuevalidation/alpha.go Show resolved Hide resolved

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 8, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 8, 2023

alexzielenski suggested changes Dec 9, 2023

View reviewed changes

alexzielenski suggested changes Dec 14, 2023

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 3, 2024

alexzielenski approved these changes Jan 3, 2024

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 3, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2024

alexzielenski force-pushed the master branch from df059ed to 51023ff Compare January 3, 2024 03:56

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2024

alexzielenski force-pushed the master branch from 51023ff to 0ee7001 Compare January 3, 2024 03:57

Schnides123 force-pushed the master branch from 0ee7001 to 43938e3 Compare January 3, 2024 04:29

Feature: Added support for OpenAPI validations marker comments

a06b517

shelved multiline marker comments for another PR

Schnides123 force-pushed the master branch from 43938e3 to a06b517 Compare January 3, 2024 05:06

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 3, 2024

alexzielenski approved these changes Jan 3, 2024

View reviewed changes

k8s-ci-robot merged commit eec4567 into kubernetes:master Jan 3, 2024
2 checks passed

alexzielenski mentioned this pull request Jan 6, 2024

Adds support for CEL marker comments #445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Added support for OpenAPI validations marker comments #435

Feature: Added support for OpenAPI validations marker comments #435

Schnides123 commented Oct 16, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 16, 2023 •

edited

Loading

k8s-ci-robot commented Oct 16, 2023

alexzielenski commented Oct 16, 2023

alexzielenski left a comment •

edited

Loading

k8s-ci-robot commented Oct 16, 2023

apelisse commented Oct 18, 2023

Schnides123 commented Dec 8, 2023

alexzielenski left a comment

alexzielenski Dec 9, 2023 •

edited

Loading

alexzielenski Jan 3, 2024

alexzielenski Dec 9, 2023

alexzielenski Jan 3, 2024

alexzielenski Dec 14, 2023

Schnides123 Dec 16, 2023

alexzielenski commented Jan 3, 2024 •

edited

Loading

alexzielenski left a comment •

edited

Loading

alexzielenski commented Jan 3, 2024 •

edited

Loading

alexzielenski commented Jan 3, 2024

alexzielenski commented Jan 3, 2024

k8s-ci-robot commented Jan 3, 2024

Feature: Added support for OpenAPI validations marker comments #435

Feature: Added support for OpenAPI validations marker comments #435

Conversation

Schnides123 commented Oct 16, 2023 • edited Loading

linux-foundation-easycla bot commented Oct 16, 2023 • edited Loading

k8s-ci-robot commented Oct 16, 2023

alexzielenski commented Oct 16, 2023

alexzielenski left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 16, 2023

apelisse commented Oct 18, 2023

Schnides123 commented Dec 8, 2023

alexzielenski left a comment

Choose a reason for hiding this comment

alexzielenski Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

alexzielenski Jan 3, 2024

Choose a reason for hiding this comment

alexzielenski Dec 9, 2023

Choose a reason for hiding this comment

alexzielenski Jan 3, 2024

Choose a reason for hiding this comment

alexzielenski Dec 14, 2023

Choose a reason for hiding this comment

Schnides123 Dec 16, 2023

Choose a reason for hiding this comment

alexzielenski commented Jan 3, 2024 • edited Loading

alexzielenski left a comment • edited Loading

Choose a reason for hiding this comment

alexzielenski commented Jan 3, 2024 • edited Loading

alexzielenski commented Jan 3, 2024

alexzielenski commented Jan 3, 2024

k8s-ci-robot commented Jan 3, 2024

Schnides123 commented Oct 16, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 16, 2023 •

edited

Loading

alexzielenski left a comment •

edited

Loading

alexzielenski Dec 9, 2023 •

edited

Loading

alexzielenski commented Jan 3, 2024 •

edited

Loading

alexzielenski left a comment •

edited

Loading

alexzielenski commented Jan 3, 2024 •

edited

Loading