Added the experimental features page#3676

slinkydeveloper · 2021-05-27T10:18:01Z

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

Proposed Changes

This adds a page to document eventing experimental features. For more details: https://docs.google.com/document/d/1AoH0FyLeuHIg5snrlCKzELQv-NEA6bbjTU2Qv3zlW5k/edit?usp=sharing and https://github.com/knative/eventing/blob/main/docs/experimental-features.md

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

netlify · 2021-05-27T10:19:41Z

✔️ Deploy Preview for dev-knative ready!

🔨 Explore the source changes: ac99766

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-knative/deploys/60b5e20eac2d760008263c61

😎 Browse the preview: https://deploy-preview-3676--dev-knative.netlify.app

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

julz · 2021-05-27T10:24:45Z

docs/eventing/experimental-features.md

+    knative.dev/config-propagation: original
+    knative.dev/config-category: eventing
+data:
+  new-cool-feature: "false"


This is really a comment on the eventing implementation of this rather than the doc, but Im only noticing this here now :).. Would it make sense (I think the proposal doc said we were going to) to align this with the serving features configmap where the settings are "enabled", "disabled", "allowed"?

We talked about that, but we decided to avoid having the opt-in (see the mail thread) features in that same CM (like you have in serving), hence this CM is only about "experimental feature on/off".

In any case, feel free to reply to that mail thread if you have some concrete proposal around it, I see your other comment below about that :). Can we ignore this discussion here for the sake of this PR?

FWIW I think some of the serving features also only have "enabled" & "disabled", so I don't think there's a reason we can't align, even if you don't have a need for an "allowed" state yet. Anyway let me go look for the email and reply there, I didn't realise there was a mail thread going on as well as the proposal doc (I've been filtering out stuff tagged [Eventing])

The original doc it had the following clause - I'm surprised after the discussion in the mailing list the doc wasn't updated and comments resolved.

We store in a config map a map of enabled/disabled features. Pretty much the same as Kubernetes Feature Gates or https://github.com/knative/serving/blob/master/pkg/apis/config/features.go

The tri-state is more expressive and allows for users to individually opt into experimental behaviour - in contrast to the operator enabling it for everyone.

This allows for a gradual rollout of features.

The tri-state is more expressive and allows for users to individually opt into experimental behaviour - in contrast to the operator enabling it for everyone.

This was discussed as well both in the doc and in the mailing list: depending on the feature, we're going to decide if we want it opt-in and opt-out by default, but because eventing has plenty of CMs, most of them already namespace scoped, it doesn't make sense to have this ConfigMap too for enabling component features.

julz · 2021-05-27T10:26:39Z

docs/eventing/experimental-features.md

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


ditto that this is really a comment on the implementation not the doc, but making it here because this is where Im seeing it -- could this be config-features to align with the same thing in knative-serving namespace? (I'd say we should rename the serving one, but that would pose a backwards-compat challenge now).

UX feedback
I think the word “experimental” should not be in any configuration to indicate the level of stability or to indicate that the configuration/feature might get removed

a better approach perhaps would be using docs to indicate this
For example a document comment in the yaml
A message on the command line like when using deprecated features in kubectl, they do this in Kubernetes know and even with colors text yellow to indicate warning
In the actual website docs in both the section explaining how to use the feature and in the Go generate rerefence html

the benefit would be that as the user upgrades from old version to new version, and it happens that the feature is stable and not longer experimental
There is not much work for the administrators to go and update the config in many places or coordinate with others to change the config in many places. Instead he can announce/doc that a new version of Knative is used and that feature X is not longer experimental and now is stable and supported

for Knative to move it out from experimental we don’t modify tests or code.

If the feature is removed from experimental then is a mute point the experience remains the same for that case as oppose feature remaining they have better experience

So this CM is an entrypoint for people that wants to try out experimental features, and when a feature is stable, the flag won't be here anymore. The name experimental is just to keep it consistent with the Experimental features process.

The CM gets commented too with details about each specific flag, and then this page will host the documentation for each flag.

For example a document comment in the yaml
A message on the command line like when using deprecated features in kubectl, they do this in Kubernetes know and even with colors text yellow to indicate warning
In the actual website docs in both the section explaining how to use the feature and in the Go generate rerefence html

For experimental features adding new fields to CRDs, we don't add them to the schema: https://github.com/knative/eventing/blob/main/docs/experimental-features.md#strategies-to-add-new-apis-for-experimental-features

So this CM is an entrypoint for people that wants to try out experimental features, and when a feature is stable, the flag won't be here anymore.

Sure but like @julz and @csantanapr have pointed out someone coming from serving to enable some functionality will now have a different approach will easily be confused and tripped up.

docs/eventing/experimental-features.md

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

docs/eventing/experimental-features.md

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

julz · 2021-05-31T08:23:00Z

/hold to make sure we're all (especially @dprotaso from an alignment with serving api version of this POV) on the same page before merging docs and potentially having people start relying on it

csantanapr · 2021-05-31T16:29:10Z

From UX I agree with a tri-state is not that a bad option, but having consistency in the project as “this is how knative handles experimental features” vs having fragmented experiences like Eventing works like x and Serving works like y is not a good UX

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

slinkydeveloper · 2021-06-01T07:32:43Z

I've updated the page per knative/eventing#5459, but I didn't added the allowed flag documentation, since we don't have any particular feature today that use it, nor I can think of any we're working on that might use the allowed state.

I suggest we document it once we have a feature that is going to use the allowed state, so we can properly describe why we need it and how to use it. My fear is that describing it now, without having a proper example for it, it will look like just confusing for the user.

julz · 2021-06-01T08:19:49Z

/unhold

abrennan89 · 2021-06-01T19:05:43Z

docs/eventing/experimental-features.md

+## Experimental features configuration
+
+When installing Eventing, the `config-features` ConfigMap is added to your cluster in the `knative-eventing` namespace.
+In order to enable a feature, you just need to add it to the config map and set its value to `"enabled"`.


Note: @eric-sap related to the other PR you're doing, it looks like it's both `` and "" 🙂 so "enabled"

In this particular case (judging by the new-cool-feature: example text below this sentence) it looks like the quotes are literally supposed to be part of the configmap (as opposed to, say, the metadata.name or namespace, which do not have quotes). I'm unsure as to whether the yaml parser cares one way or the other. I only mention it here because in the config-kafka configmap, there are no fields that require literal double-quotes around them (nor are there examples of such in the document).

That's my bad, i was using true and false as values before, which needs escapes. #3714

abrennan89 · 2021-06-01T19:06:53Z

docs/eventing/experimental-features.md

+```yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: config-features
+  namespace: knative-eventing
+  labels:
+    eventing.knative.dev/release: devel
+    knative.dev/config-propagation: original
+    knative.dev/config-category: eventing
+data:
+  new-cool-feature: "enabled"
+```


Can you add the kubectl command part as per the style guide for any YAML files

I don't know if there's a kubectl counterpart for modifying the config maps...

docs/eventing/experimental-features.md

abrennan89 · 2021-06-01T19:07:59Z

mkdocs.yml

      - Debugging: eventing/debugging/index.md
      - Accessing CloudEvent traces: eventing/accessing-traces.md
      - Metrics API: eventing/metrics.md
+      - Experimental Features: eventing/experimental-features.md


Should probably go in the admin guide if they all require configmap editing?

🤷 I see it's the same for serving...

abrennan89

Couple of comments

csantanapr · 2021-06-03T01:22:42Z

/lgtm
thanks for addressing my comments @slinkydeveloper

knative-prow-robot · 2021-06-03T01:23:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csantanapr, 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~~ [csantanapr]

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

csantanapr · 2021-06-03T11:43:52Z

don't know why this merged when the PR had a "Request changes" from @abrennan89

slinkydeveloper · 2021-06-03T12:17:27Z

@csantanapr 🤷, I'm addressing the comments in this other PR: #3714

* Added the experimental features page Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Trailing whitespaces Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Added to the index Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * suggestion by julz Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * suggestion by ashleigh Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Reflected changes of knative/eventing#5459 Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Added the experimental features page

992da33

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label May 27, 2021

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 27, 2021

slinkydeveloper added 2 commits May 27, 2021 12:20

Trailing whitespaces

844cfa4

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

Added to the index

47f78a8

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

julz reviewed May 27, 2021

View reviewed changes

suggestion by julz

8730426

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

abrennan89 reviewed May 27, 2021

View reviewed changes

docs/eventing/experimental-features.md Outdated Show resolved Hide resolved

suggestion by ashleigh

87616af

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

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

slinkydeveloper mentioned this pull request Jun 1, 2021

Added tristate flags and changed experimental config map name knative/eventing#5459

Merged

5 tasks

Reflected changes of knative/eventing#5459

ac99766

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

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

slinkydeveloper requested review from abrennan89 and csantanapr June 1, 2021 16:05

abrennan89 reviewed Jun 1, 2021

View reviewed changes

docs/eventing/experimental-features.md Show resolved Hide resolved

abrennan89 reviewed Jun 1, 2021

View reviewed changes

abrennan89 suggested changes Jun 1, 2021

View reviewed changes

knative-prow-robot assigned abrennan89 Jun 1, 2021

knative-prow-robot assigned csantanapr Jun 3, 2021

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

csantanapr approved these changes Jun 3, 2021

View reviewed changes

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

knative-prow-robot merged commit b40c7df into knative:mkdocs Jun 3, 2021

slinkydeveloper deleted the experimental-features branch June 3, 2021 07:17

slinkydeveloper mentioned this pull request Jun 3, 2021

Remove double quotes on enabled and disabled #3714

Merged

slinkydeveloper mentioned this pull request Jun 15, 2021

Add a warning about experimental features #3803

Merged

Conversation

slinkydeveloper commented May 27, 2021

Proposed Changes

Uh oh!

netlify bot commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julz May 27, 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!

slinkydeveloper May 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csantanapr May 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

julz commented May 31, 2021

Uh oh!

csantanapr commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

julz commented Jun 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrennan89 left a comment

Choose a reason for hiding this comment

Uh oh!

csantanapr commented Jun 3, 2021

Uh oh!

knative-prow-robot commented Jun 3, 2021

Uh oh!

csantanapr commented Jun 3, 2021

Uh oh!

slinkydeveloper commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

netlify bot commented May 27, 2021 •

edited

Loading

julz May 27, 2021 •

edited

Loading

slinkydeveloper May 27, 2021 •

edited

Loading

csantanapr May 27, 2021 •

edited

Loading

csantanapr commented May 31, 2021 •

edited

Loading

slinkydeveloper commented Jun 1, 2021 •

edited

Loading