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.

Sign up for GitHub

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

Jump to bottom

Add crdschema generator #183

Merged
merged 5 commits into from
Aug 29, 2019
Merged

Add crdschema generator #183

merged 5 commits into from
Aug 29, 2019

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Apr 11, 2019 •
edited
Loading

While the crd generator creates complete CRD manifests from tags, the crdschema generator is much less opinionated and only updates validation schemas in a given directory of CRD manifests.

# Generate OpenAPI v3 schemas for API packages and merge them into existing CRD manifests
$ controller-gen crdschema paths=./pkg/apis/... crdschema:manifests=./manifests output:dir=./manifests

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 11, 2019
@k8s-ci-robot k8s-ci-robot requested review from droot and justinsb April 11, 2019 07:59
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 11, 2019
@sttts
Copy link
Contributor Author

sttts commented Apr 11, 2019

/assign @DirectXMan12 @damemi @droot

@k8s-ci-robot
Copy link
Contributor

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

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @DirectXMan12 @damemi @droot

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.

@sttts sttts force-pushed the sttts-crd-schema-gen branch 2 times, most recently from f9cf6cb to 2535f8f Compare April 11, 2019 09:57
@damemi damemi mentioned this pull request Apr 11, 2019
Merged
@DirectXMan12
Copy link
Contributor

We've actually been working on a similar refactoring that might help you, because other users want to be able to generate stand-alone validation schemas. Hopefully that should make this easier.

As it stands, this code is a bit too hacky to merge, IMO, but the refactor should make it much nicer.

/hold

@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 Apr 11, 2019
@DirectXMan12
Copy link
Contributor

discussed a bit more with @sttts

TL;DR: I like the idea of being able to generate a schema by itself (we actually have other people that want it too). Generating a stand-alone schema, or a schema as a patch, should be fine. Any patching should probably just be done using kustomize instead. @mengqiy is working on a refactor that should make this less hacky.

cc @damemi as well

@sttts sttts force-pushed the sttts-crd-schema-gen branch 2 times, most recently from 4b5814e to 5bd4a20 Compare May 8, 2019 16:22
@sttts sttts changed the title WIP: Add crd-schema-gen Add crd-schema-gen May 8, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2019
@sttts
Copy link
Contributor Author

sttts commented May 8, 2019

@DirectXMan12 @damemi ported this to the new generator pattern.

@sttts sttts force-pushed the sttts-crd-schema-gen branch from 5bd4a20 to eef7dc0 Compare May 8, 2019 16:32
sttts
sttts commented May 8, 2019
View reviewed changes
cmd/crd-schema-gen/main.go Outdated Show resolved Hide resolved
sttts
sttts commented May 8, 2019
View reviewed changes
pkg/crd-schema/yaml/nested.go Outdated Show resolved Hide resolved
@sttts sttts force-pushed the sttts-crd-schema-gen branch 7 times, most recently from 96bedf6 to d0d9d53 Compare May 8, 2019 20:57
@LorbusChris
Copy link

@DirectXMan12 @damemi @droot could you have another look?
This is a really very useful feature for us.

@sttts sttts force-pushed the sttts-crd-schema-gen branch from f5b6a90 to f210224 Compare August 2, 2019 15:42
@s-urbaniak
Copy link

👍 on this, this would help us a lot in many operators doing schema generation.

@xmudrii
Copy link
Member

xmudrii commented Aug 2, 2019

+1
This would be a very useful feature 💯

@DirectXMan12
Copy link
Contributor

yeah, I'm super-sorry about this one. I've just been really swamped recently, and it kinda fell off my radar.

@DirectXMan12 DirectXMan12 force-pushed the sttts-crd-schema-gen branch from f210224 to 300e96e Compare August 7, 2019 23:53
@DirectXMan12
Copy link
Contributor

ok, I've pushed some changes to this that bring it to a position where I'm more comfortable merging this (I'm still a bit uncomfortable with all the raw YAML manipulation, but it's hard to avoid that with your usecase constraints).

Namely:

  • fixed the tests to use the proper testdata directory
  • updated to use yaml.v3 node trees, which preserve style, comments, etc and make the path traversal a bit cleaner
  • removed the diffing logic, on the presumption that using diff is ok. If this isn't true, please do let me know, but it seemed like there were too many things rolled into one here. We could maybe add a generic diffing output rule
  • made CRD writes unconditional. yaml.v3 should make unchanged writes a noop (modulo some whitespace changes). Please lmk if this is a problem.

The "no reordering of maps" properties are still preserved, as are comments and flow vs block style.

I'd like to have @droot take a quick look at this too, since I've refactored a bunch of stuff myself.
If @sttts or anyone else familiar with the usecase constraints on this thread can also take a look, I would appreciate it. I'm trying to come to an arrangement here so that we can ensure this code fits in a bit better with the overall vision on controller-tools -- as submitted, it didn't decompose well, and pulled in a number of different tasks into a single, bespoke tool.

@DirectXMan12 DirectXMan12 force-pushed the sttts-crd-schema-gen branch 2 times, most recently from 5e8ebd1 to 4452725 Compare August 8, 2019 21:00
sttts
sttts commented Aug 12, 2019
View reviewed changes
pkg/schemapatcher/gen.go Outdated
// - It needs to make "stable" changes that don't mess with map key ordering
// (in order to facilitate validating that no change has occurred)
// - It needs to collapse identical schema versions into a top-level schema,
// if all versions are identical
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not special for schemapatcher, but a general requirement for multi-version, schemaful CRDs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll make this clearer in the comment -- I was mainly just trying to motivate why "stick the output of parser.NeedFlattenedSchema into a single JSON patch operation" is insufficient.

@sttts
Copy link
Contributor Author

sttts commented Aug 12, 2019

@DirectXMan12 your rewrite looks good.

I am having problems to run the tests though, and also to run the binary (probably the same issue):

$ controller-gen schemapatch paths=./vendor/github.com/openshift/api/... schemapatch:manifests=./manifests output:dir=./manifests
Error: unable to parse option "schemapatch": [missing argument "manifests"]

droot
droot reviewed Aug 12, 2019
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

looks good to me.

cmd/controller-gen/main.go Outdated
@@ -134,6 +136,9 @@ func main() {
# Generate deepcopy/runtime.Object implementations for a particular file
controller-gen object paths=./apis/v1beta1/some_types.go

# Generate OpenAPI v3 schemas for API packages and merge them into existing CRD manifests
controller-gen crdschema paths=./pkg/apis/... crdschema:manifests=./manifests output:dir=./manifests
Copy link
Contributor

Choose a reason for hiding this comment

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

s/crdschema/schemapatch

@DirectXMan12
Copy link
Contributor

whoops, didn't actually get merged here. Will fix this up this week and get this merged.

sttts and others added 5 commits August 29, 2019 07:56
@sttts @DirectXMan12
Add crdschema generator
584080e
@sttts @DirectXMan12
dep ensure -v
17fc055
@sttts @DirectXMan12
GO111MODULE=on go mod tidy
07643d5
@DirectXMan12
allow easy access to flattened crds from parser
8c38549
@DirectXMan12
Bring schemapatcher in line with CT coding style
3687756 
This updates schemapatcher (formerly crdschema) rather drastically,
reducing its scope a bit and cleaning up its code to bring it in line
with the rest of controller-tools.

Functionality changes:
- change verification is removed -- use diff
- yaml.v3 is used, which should preserve comments, style, etc
- CRDs are written unconditionally.  Doing this for unchanged CRDs
  should be a no-op, modulo slight whitespace changes.
@DirectXMan12
Copy link
Contributor

ok, fixed up now. The reason your invocation didn't work is because you've got an extra crdschema at the beginning.

@DirectXMan12
Copy link
Contributor

tested against the openshift cluster network operator repo too, just in case.

@DirectXMan12
Copy link
Contributor

and now, the rare, arcane, not-quite-self-approval, a secret technique pass down from ancient aeons:

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, sttts

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 Aug 29, 2019
@DirectXMan12
Copy link
Contributor

/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 Aug 29, 2019
@k8s-ci-robot k8s-ci-robot merged commit 58324d7 into kubernetes-sigs:master Aug 29, 2019
@sttts
Copy link
Contributor Author

sttts commented Aug 29, 2019

@DirectXMan12 this is fantastic news! Thanks for finishing this up and get it merged 🎉🚀

@s-urbaniak s-urbaniak mentioned this pull request Nov 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy left review comments

@droot droot droot left review comments

@DirectXMan12 DirectXMan12 DirectXMan12 requested changes

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@DirectXMan12 DirectXMan12

@droot droot

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sttts @k8s-ci-robot @DirectXMan12 @mengqiy @LorbusChris @s-urbaniak @xmudrii @droot