Add complete schemas to all CRDs by markusthoemmes · Pull Request #11244 · knative/serving

markusthoemmes · 2021-04-21T13:39:41Z

Proposed Changes

Fixes #912

This is a stepping stone towards generating schemas for all of our CRDs. Generating the CRDs is not yet part of update-codegen.sh since we need to put the actual machinery in place through the hack repository, potentially forking the respective tooling to our repositories first.

I thought this might still be useful. Granted, this is not based on any accepted concept right now, so I'm happy to bin it all.

Release Note

Added schemas to all CRDs.

markusthoemmes · 2021-04-21T13:41:05Z

/hold

There's a vendor change in here that needs to be done in pkg first.

codecov · 2021-04-21T13:45:29Z

Codecov Report

Merging #11244 (d5eb000) into main (28ba542) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #11244      +/-   ##
==========================================
+ Coverage   87.65%   87.72%   +0.06%     
==========================================
  Files         191      191              
  Lines        9187     9186       -1     
==========================================
+ Hits         8053     8058       +5     
+ Misses        880      875       -5     
+ Partials      254      253       -1

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_types.go	`100.00% <ø> (ø)`
pkg/reconciler/revision/background.go	`95.06% <0.00%> (-0.12%)`	⬇️
pkg/reconciler/revision/controller.go	`85.41% <0.00%> (+0.31%)`	⬆️
pkg/reconciler/configuration/configuration.go	`88.28% <0.00%> (+1.56%)`	⬆️
pkg/activator/net/revision_backends.go	`93.30% <0.00%> (+1.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28ba542...d5eb000. Read the comment docs.

chaodaiG · 2021-04-23T15:18:52Z

/retest

markusthoemmes · 2021-04-23T15:39:15Z

/hold

This got bigger now 😂

evankanderson

I'm assuming that the entirety of the CRD definition is being created by the controller-gen binary, and we're not needing to do part-and-part copying?

julz · 2021-04-26T11:18:17Z

/lgtm

markusthoemmes · 2021-04-26T17:47:54Z

This is ready from my perspective

/unhold

markusthoemmes · 2021-04-26T18:07:22Z

/assign @dprotaso @n3wscott

joshuawilson · 2021-04-27T13:44:33Z

Nice to see this @markusthoemmes, thanks for all the hard work.

dprotaso · 2021-04-28T13:54:14Z

 // RevisionTemplateSpec describes the data a revision should have when created from a template.
 // Based on: https://github.com/kubernetes/api/blob/e771f807/core/v1/types.go#L3179-L3190
 type RevisionTemplateSpec struct {
+	// +kubebuilder:pruning:PreserveUnknownFields


To clarify this notation applies to the fields in metav1.ObjectMeta ? Is there a reason why we're allowing unknown fields here?

Does your patch mean this annotation is no longer necessary kubernetes-sigs/controller-tools#563 ?

Tbh, this was kind of a stopgap solution because stuff has been failing without it. We currently print out deletionTimestamp: null to every nested ObjectMeta and just the schema would cause this to drop, so I thought the problem might be around that. Hence I added this for compatiblity.

dprotaso · 2021-04-28T13:59:01Z

+# 2. There's a missing feature to properly generate nested ObjectMeta
+#    see https://github.com/kubernetes-sigs/controller-tools/pull/563
+# 3. We need specialized logic to filter down the surface of PodSpec we allow in Knative.
+controller-gen schemapatch:manifests=config/core/300-resources \


can you do the wrangling of cloning/installing your repo in a tmp-dir and updating the path so folks can update the schema without going through those manual hoops

I can take a whack at that, though I'd like to priotize the schema itself for now so we can get that baking. There'll be incremental improvements to the devx anyway as the goal ultimately is to have it run via update-codegen.sh.

dprotaso · 2021-04-28T14:06:12Z

note to other reviewers: x-kubernetes-preserve-unknown-fields: true only applies to sibling properties of a node and not the entire subtree

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#controlling-pruning

dprotaso · 2021-04-30T00:18:38Z

/lgtm
/approve

knative-prow-robot · 2021-04-30T00:18:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~config/OWNERS~~ [dprotaso]
~~hack/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 21, 2021

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Apr 21, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2021

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2021

markusthoemmes force-pushed the add-schema-min branch from 55ff3f3 to 76f068c Compare April 22, 2021 05:53

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2021

markusthoemmes closed this Apr 23, 2021

markusthoemmes reopened this Apr 23, 2021

markusthoemmes mentioned this pull request Apr 23, 2021

Incorporate schema into CRDs #912

Closed

evankanderson reviewed Apr 23, 2021

View reviewed changes

Comment thread config/core/300-resources/metric.yaml

julz reviewed Apr 26, 2021

View reviewed changes

Comment thread hack/update-codegen.sh Outdated

knative-prow-robot assigned julz Apr 26, 2021

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Apr 26, 2021

markusthoemmes changed the title ~~Setup CRD generation machinery and generate initial schemas~~ Setup CRD generation machinery and generate schemas Apr 26, 2021

julz reviewed Apr 26, 2021

View reviewed changes

Comment thread config/core/300-resources/route.yaml

Comment thread hack/update-schemas.sh Outdated

dprotaso reviewed Apr 26, 2021

View reviewed changes

Comment thread config/core/300-resources/revision.yaml Outdated

markusthoemmes added 6 commits April 26, 2021 19:45

Setup CRD generation machinery and generate initial schemas

1fdde36

Add filtered PodSpec schema

d6e3397

Fix embedded types

7afdda1

Fix things

acdc9c9

Allow preservation for some more types

4fff07a

Add comments

01d9ed8

markusthoemmes added 5 commits April 26, 2021 19:45

Add an explicit comment towards our patched version

dfe8b34

Detach update-codegen from schema generation for now

761c04a

Drop incorrect default descriptions

411c06a

Drop unnecessary git based rollback

b7fd161

Simplify further

9cd1ecf

markusthoemmes force-pushed the add-schema-min branch from 0b262e3 to 9cd1ecf Compare April 26, 2021 17:46

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2021

Drop now unnecessary comment

5d85d52

knative-prow-robot assigned dprotaso and n3wscott Apr 26, 2021

markusthoemmes mentioned this pull request Apr 27, 2021

Add the Spec fields to the service schema #10003

Closed

markusthoemmes changed the title ~~Setup CRD generation machinery and generate schemas~~ Add complete schemas to all CRDs Apr 27, 2021

dprotaso reviewed Apr 28, 2021

View reviewed changes

markusthoemmes added 4 commits April 28, 2021 17:06

Reorder schema and manual fields

bddf483

Drop feature flagged fields

2ab0ed6

Remove fat fingering

8e40ee5

Fix leftovers

d5eb000

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2021

knative-prow-robot merged commit 101c6f1 into knative:main Apr 30, 2021

Conversation

markusthoemmes commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

markusthoemmes commented Apr 21, 2021

Uh oh!

codecov Bot commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chaodaiG commented Apr 23, 2021

Uh oh!

markusthoemmes commented Apr 23, 2021

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

julz commented Apr 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markusthoemmes commented Apr 26, 2021

Uh oh!

markusthoemmes commented Apr 26, 2021

Uh oh!

joshuawilson commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

dprotaso Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Apr 28, 2021

Uh oh!

dprotaso commented Apr 30, 2021

Uh oh!

knative-prow-robot commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

markusthoemmes commented Apr 21, 2021 •

edited

Loading

codecov Bot commented Apr 21, 2021 •

edited

Loading