Experimental features configuration and validation by slinkydeveloper · Pull Request #5214 · knative/eventing

slinkydeveloper · 2021-04-08T13:20:26Z

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Details: https://docs.google.com/document/d/1AoH0FyLeuHIg5snrlCKzELQv-NEA6bbjTU2Qv3zlW5k/edit?usp=sharing

Proposed Changes

🎁 Store to read the experimental features flags from a specific config map
🎁 Validator for APIs when using experimental features
🎁 Scaffolded the directory to add experimental features e2e/conformance tests

Release Note

Docs

knative-prow-robot · 2021-04-08T13:20:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

matzew · 2021-04-08T13:50:38Z

How do I register "experimental features"?

slinkydeveloper · 2021-04-08T13:52:22Z

How do I register "experimental features"?

There is no need to "register" an experimental feature:

By default, all features are disabled.
If the CM contains my-feature: "true", then the feature is enabled.

matzew · 2021-04-08T13:59:31Z

If the CM contains my-feature: "true", then the feature is enabled.

not fully getting how the my-feature is than really defined, as like more concrete: timeout-enabeld?

how would that apply to a timeout field?

slinkydeveloper · 2021-04-08T14:02:23Z

@matzew the field still has to be in the api data structure, but it will be rejected by the webhook if the feature is not enabled. I'll explain it in the doc I'm writing 😄

lionelvillard · 2021-04-08T14:09:26Z

@@ -0,0 +1 @@
+core/configmaps/experimental-features.yaml


Can we call this just features.yaml, to be consistent with Knative Serving? A feature can be Alpha, ie experimental. A feature can also be stable and still be disabled if a vendor doesn't want it for X reason.

I added a comment in the doc about that. I don't like mixing up the concepts, I would prefer that if a feature still needs a flag to enable/disable, it even after GA, that flag should live somewhere else, wherever it makes more sense to do so. e.g. a feature related to broker config should live in the broker config map.

This approach of keeping it separate from experimental features also fits in the various scoping of configurations we already implement for every component. I don't want experimental features to end up re-doing that.

lionelvillard · 2021-04-08T14:09:35Z

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: config-experimental-features


lionelvillard · 2021-04-08T14:10:20Z

+
+			if (fieldVal.Kind() == reflect.Ptr || fieldVal.Kind() == reflect.Slice || fieldVal.Kind() == reflect.Map) && !fieldVal.IsNil() {
+				errs = errs.Also(&apis.FieldError{
+					Message: fmt.Sprintf("Disallowed field because the experimental feature '%s' is disabled", featureName),


remove experimental

codecov · 2021-04-08T14:15:44Z

Codecov Report

Merging #5214 (f093286) into main (0653ac4) will increase coverage by 0.08%.
The diff coverage is 90.76%.

@@            Coverage Diff             @@
##             main    #5214      +/-   ##
==========================================
+ Coverage   82.68%   82.77%   +0.08%     
==========================================
  Files         194      197       +3     
  Lines        6006     6071      +65     
==========================================
+ Hits         4966     5025      +59     
- Misses        717      721       +4     
- Partials      323      325       +2

Impacted Files	Coverage Δ
pkg/apis/experimental/features.go	`84.61% <84.61%> (ø)`
pkg/apis/experimental/store.go	`91.30% <91.30%> (ø)`
pkg/apis/experimental/api_validation.go	`93.10% <93.10%> (ø)`

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 0653ac4...f093286. Read the comment docs.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

…conformance tests Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2021-05-19T09:41:48Z

I've added a directory (and modified the GH action) where we can place the experimental features e2e/conformance tests. These are not going to cause any troubles to existing e2e/conformance tests.

Fix e2e tests Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-metrics-robot · 2021-05-19T09:56:30Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/experimental/api_validation.go	Do not exist	95.5%
pkg/apis/experimental/features.go	Do not exist	92.3%
pkg/apis/experimental/store.go	Do not exist	93.8%

vaikas · 2021-05-24T18:08:29Z

/lgtm
Thanks for driving this through. Will be great to keep on experimenting and improving and learning!
I'll wait on actual approval, since you said you'd do that tmw.

matzew

/lgtm
/approve

knative-prow-robot · 2021-05-26T09:03:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, slinkydeveloper

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:

~~OWNERS~~ [matzew,slinkydeveloper]

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

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2021

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2021

slinkydeveloper marked this pull request as ready for review April 8, 2021 13:20

lionelvillard reviewed Apr 8, 2021

View reviewed changes

Comment thread pkg/apis/experimental/api_validation.go

lionelvillard reviewed Apr 8, 2021

View reviewed changes

knative-prow-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 Apr 8, 2021

slinkydeveloper changed the title ~~[WIP] Experimental features configuration and validation~~ Experimental features configuration and validation Apr 22, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2021

slinkydeveloper mentioned this pull request Apr 22, 2021

Initial attempt at 429 Retry-After header support #5285

Closed

slinkydeveloper mentioned this pull request May 17, 2021

Experimental features process documentation #5404

Merged

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. area/test-and-release Test infrastructure, tests or release labels May 19, 2021

slinkydeveloper added 6 commits May 19, 2021 11:38

Experimental features

d9d757e

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

godoc

86dd450

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

ValidateAnnotations

41d8599

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Copyright

e9facb5

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Now the ValidateAPIField can validate nested fields

c946a5d

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Added the directory and the action for the experimental features e2e/…

fbf82c2

…conformance tests Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper force-pushed the experimental_flags branch from 19ead48 to fbf82c2 Compare May 19, 2021 09:39

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

Removed custom build tag and using just e2e tag

f093286

Fix e2e tests Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot assigned vaikas May 24, 2021

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

matzew approved these changes May 26, 2021

View reviewed changes

knative-prow-robot assigned matzew May 26, 2021

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

knative-prow-robot merged commit 295134c into knative:main May 26, 2021

slinkydeveloper deleted the experimental_flags branch May 26, 2021 12:31

		@@ -0,0 +1 @@
		core/configmaps/experimental-features.yaml No newline at end of file

Conversation

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

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Apr 8, 2021

Uh oh!

matzew commented Apr 8, 2021

Uh oh!

slinkydeveloper commented Apr 8, 2021

Uh oh!

matzew commented Apr 8, 2021

Uh oh!

slinkydeveloper commented Apr 8, 2021

Uh oh!

lionelvillard Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionelvillard Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lionelvillard Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

slinkydeveloper commented May 19, 2021

Uh oh!

knative-metrics-robot commented May 19, 2021

Uh oh!

vaikas commented May 24, 2021

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

slinkydeveloper commented Apr 8, 2021 •

edited

Loading

slinkydeveloper Apr 9, 2021 •

edited

Loading

codecov Bot commented Apr 8, 2021 •

edited

Loading