Skip to content

Experimental features process documentation#5404

Merged
knative-prow-robot merged 7 commits into
knative:mainfrom
slinkydeveloper:experimental_features_process
May 26, 2021
Merged

Experimental features process documentation#5404
knative-prow-robot merged 7 commits into
knative:mainfrom
slinkydeveloper:experimental_features_process

Conversation

@slinkydeveloper
Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper commented May 17, 2021

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

This is a transcript of https://docs.google.com/document/d/1AoH0FyLeuHIg5snrlCKzELQv-NEA6bbjTU2Qv3zlW5k/edit?usp=sharing.

Blocked by #5214, because it contains some technical details I want to specify in the doc and link the appropriate code.

Proposed Changes

  • 🎁 Experimental features process documentation

@google-cla google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label May 17, 2021
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 17, 2021
@slinkydeveloper slinkydeveloper changed the title Experimental features process documentation [WIP] Experimental features process documentation May 17, 2021
@knative-prow-robot 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 May 17, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2021

Codecov Report

Merging #5404 (0362044) into main (295134c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5404   +/-   ##
=======================================
  Coverage   82.72%   82.72%           
=======================================
  Files         197      197           
  Lines        6056     6056           
=======================================
  Hits         5010     5010           
  Misses        722      722           
  Partials      324      324           

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 295134c...0362044. Read the comment docs.

Comment thread docs/experimental-features.md Outdated
Comment thread docs/experimental-features.md Outdated
Comment thread docs/experimental-features.md Outdated
Comment thread docs/experimental-features.md Outdated
[experimental.ValidateAPIFields()](https://github.com/knative/eventing/blob/917f57f0e68abbd0256c5fa4bff1ce2e83ffe78b/pkg/apis/experimental/api_validation.go#L13)
- Alternative to the above approach, If the feature affects a whole Resource,
and the API is a single value, that is not an object or an array of values,
use an annotation. Then, in the webhook, you can reject resources with
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, the choice between annotations vs spec field(s) depends on whether the feature is normative or not. An annotation might graduate into a field if it gains general consensus and applicability.

Copy link
Copy Markdown
Contributor Author

@slinkydeveloper slinkydeveloper May 19, 2021

Choose a reason for hiding this comment

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

For the clarification between normative and non-normative, see comment below.

IMO, the choice between annotations vs spec field(s) depends on whether the feature is normative or not.

Any suggestions to reword?

Comment thread docs/experimental-features.md Outdated

- Disabled by default.
- No vendor is required to implement the new feature at this point, but it’s
encouraged to.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All depends on whether the feature is normative or not. A vendor is never required to implement a feature, unless the feature is normative and the vendor wants to be conformant wrt the new feature.

Maybe drop this line and say by definition an experimental feature is not normative so vendors don't have to support it in order to be compliant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In this commit 9536f95 I've added some clarifications between normative and non-normative. I think this document should not specify how to choose between normative and non-normative, because IMO depends on the feature itself and the discussions around it. I've just clarified that we cover both. How does it look like?

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper changed the title [WIP] Experimental features process documentation Experimental features process documentation May 26, 2021
@knative-prow-robot 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 May 26, 2021
@slinkydeveloper slinkydeveloper force-pushed the experimental_features_process branch from 9089731 to 0362044 Compare May 26, 2021 13:08
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021
@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

Rebased and added the links to the code, now it's ready to go!

@lionelvillard
Copy link
Copy Markdown
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2021
@knative-prow-robot knative-prow-robot merged commit 6776718 into knative:main May 26, 2021
@slinkydeveloper slinkydeveloper deleted the experimental_features_process branch May 26, 2021 13:50
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. cla: yes Indicates the PR's author has signed the CLA. lgtm 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.

3 participants