Updating validation tests to follow the spec for `spec.subscriber` by salaboy · Pull Request #5762 · knative/eventing

salaboy · 2021-09-28T08:48:48Z

Proposed Changes

🧹 Update or clean up current behavior
This Pull Request updates the validation of Subscription resources requiring the spec.subscriber to be set for all subscriptions. This is based on the spec update linked in #5756 .

This PR removes 2 old validation tests which were creating previously valid Subscriptions without spec.subscriber. These tests are no longer needed.

This PR also removes the resource validation of the spec.reply property as it is no longer required by the spec.

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

This change brake compatibility with previous versions as it enforces the presence of `spec.subscriber` for Subscriptions resources. This is based on a change in the spec to improve the user experience when using Subscriptions.

Docs

salaboy · 2021-09-28T08:52:05Z

@lionelvillard @evankanderson folks if you can also point me to where in the docs this should be updated that will be great .. I am happy to send a PR there if I find where this change is relevant.

evankanderson

I'm just realizing this doesn't include a flag / controller environment variable to bridge over the 0.26 and prior behavior.

See https://github.com/knative/eventing/blob/main/pkg/apis/duck/v1/delivery_types.go#L79 for an example of a flag-controlled validation.

salaboy · 2021-09-29T08:15:07Z

@evankanderson regarding the flag, I can see that for features, but for this change.. what is the expected behavior?
should it be something like:

if !<flag>
  <old behavior> // this requires me to re-add the old behavior, and keep the tests for that old behavior
else
  <new behavior>

To achieve this, we need to have the old logic and the new logic as well and we need a new flag. I couldn't find anything related to versions.. and it feels bad to create a flag called "ConformantToNewSpec".
Also, do we really need to keep the old behavior/nonspec compliant? for how long? I do realize that this is a breaking change.

@lionelvillard @evankanderson Can you share some advice?

PS: I see that there are some tests failing still.. but let's settle on how this should work so I can go and fix those tests.

lionelvillard · 2021-09-29T18:15:42Z

@salaboy the reference doc is generated from https://github.com/knative/eventing/blob/main/pkg/apis/messaging/v1/subscription_types.go#L97

I quickly look at the rest of the documentation and I didn't find anything else.

lionelvillard · 2021-09-29T18:35:41Z

IMO, the way to incrementally get Knative eventing conformant is to add a new experimental flag. It's not ideal, since this is not an experimental feature, but at least it gives you a framework to work with. I propose to call the flag strict-subscriber but feel free to change to something that feel more natural to you.

Here a good example of how to add a new experimental flag: #5440

Proposed timeline:

release 0.27: "Alpha", feature disabled by default. At this stage, Knative eventing is conformant when the flag is enabled.
release 0.28: "Beta": feature is enabled by default, can be disabled. At this stage, Knative eventing is conformant by default
release 0.X: GA. feature cannot be disabled.

X is the release number in 1 year (~35?)

WDYT?

salaboy · 2021-09-30T07:43:54Z

@lionelvillard after thinking about this overnight, I woke up decided to implement the flag. Your explanation makes my life way much easier and thanks for the references. I will be working on this today and tomorrow hopefully I will have an updated PR by then.

salaboy · 2021-09-30T07:44:40Z

By the way.. you also solved the naming problem (strict-subscriber) so you rock 🤘 !

salaboy · 2021-09-30T11:04:33Z

@lionelvillard I've added the feature flag, disabled by default and I have a test testing a bunch of scenarios with strict-subscriber.

Let me wait for CI to see which tests are failing

codecov · 2021-09-30T11:07:38Z

Codecov Report

Merging #5762 (8f4edda) into main (ce818dc) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5762      +/-   ##
==========================================
- Coverage   82.52%   82.41%   -0.11%     
==========================================
  Files         203      204       +1     
  Lines        6374     6460      +86     
==========================================
+ Hits         5260     5324      +64     
- Misses        768      778      +10     
- Partials      346      358      +12

Impacted Files	Coverage Δ
pkg/apis/messaging/v1/subscription_validation.go	`87.50% <100.00%> (+2.50%)`	⬆️
...nnelfanout/multi_channel_fanout_message_handler.go	`83.78% <0.00%> (-7.40%)`	⬇️
...reconciler/inmemorychannel/dispatcher/readiness.go	`71.42% <0.00%> (ø)`
pkg/broker/filter/filter_handler.go	`80.66% <0.00%> (+0.12%)`	⬆️
pkg/resolver/mapping_resolver.go	`73.46% <0.00%> (+0.55%)`	⬆️
pkg/channel/message_dispatcher.go	`79.68% <0.00%> (+0.65%)`	⬆️
pkg/reconciler/parallel/parallel.go	`60.00% <0.00%> (+1.17%)`	⬆️
pkg/apis/messaging/v1/channel_validation.go	`84.21% <0.00%> (+1.35%)`	⬆️
...econciler/inmemorychannel/dispatcher/controller.go	`88.88% <0.00%> (+1.38%)`	⬆️
... and 1 more

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 ce818dc...8f4edda. Read the comment docs.

evankanderson · 2021-09-30T13:14:23Z

We don't have e2e tests for the new behavior, but it's currently off by default.

I'm fine with that in this PR and then subsequently trying to convince Lionel to skip the alpha phase for this flag and adding e2e tests then if he agrees.

So,

/lgtm

from me, but I'm going to let Lionel approve when he's happy. (At the very least, I think checking this in as flag-off-by-default is the right choice for this PR to enable small incremental progress.)

lionelvillard · 2021-09-30T13:55:02Z

I restarted the kind actions, to see if the failure was a flake.

lionelvillard · 2021-09-30T14:10:54Z

/lgtm

then subsequently trying to convince Lionel to skip the alpha phase

I'm worried that if we skip the alpha phase we risk breaking existing applications, in particular the ones that dynamically create Subscription objects. If we find a solution to this problem, then yes I think we can skip the alpha phase.

lionelvillard · 2021-09-30T14:35:30Z

/hold

You also need to add the feature in this configmap: https://github.com/knative/eventing/blob/main/config/core/configmaps/features.yaml

We may want to document it here: https://knative.dev/docs/eventing/experimental-features/. Do you mind opening an issue in the docs repo?

salaboy · 2021-09-30T16:27:34Z

@lionelvillard better than an issue is a PR: knative/docs#4311
PR sent to the docs and flag added to yaml

lionelvillard · 2021-10-01T12:28:16Z

@salaboy great! Can you make the linter happy and then it should be ready to be merged. Thanks!

pierDipi · 2021-10-13T13:53:55Z

@salaboy ^

salaboy · 2021-10-13T15:13:15Z

@pierDipi sorry.. I don't know why I missed this one.. linter should be happy now.

knative-metrics-robot · 2021-10-13T15:27:32Z

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	Delta
pkg/apis/messaging/v1/subscription_validation.go	97.3%	97.6%	0.3

pierDipi · 2021-10-13T15:37:25Z

/cc @lionelvillard @evankanderson

Can you please review this again?

evankanderson

/lgtm
/approve

knative-prow-robot · 2021-10-13T15:42:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, salaboy

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~~ [evankanderson]

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

pierDipi · 2021-10-13T16:42:25Z

/unhold

Fixes #5756 As per knative/specs#30 the spec was changed from requiring one of `spec.subscriber` or `spec.reply` to be set to require that `spec.subscriber` is set. As part of this change the `strict-subscriber` feature flag was introduced to disable this validation (#5762) and promoted to beta (enabled by default) in 1.7 (#6473). :broom: This commit removes this feature flag again so the validation can't be disabled anymore. ## Proposed Changes * Remove `strict-subscriber` feature flag * Promote strict subscriber validation to GA ### Pre-review Checklist - [ ] **At least 80% unit test coverage** - [ ] **E2E tests** for any new behavior - [ ] **Docs PR** for any user-facing impact - [ ] **Spec PR** for any new API feature - [ ] **Conformance test** for any change to the spec **Release Note** ```release-note Remove the possibility to disable strict subscriber validation. When the reply field is specified without a subscriber, the reply field won't be used as a subscriber by default and the subscriber validation will fail. ``` Signed-off-by: Christoph Stäbler <cstabler@redhat.com>

updating validation tests to follow the spec

a263de6

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

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

goimports

89220cb

evankanderson reviewed Sep 28, 2021

View reviewed changes

Comment thread pkg/apis/messaging/v1/subscription_validation.go Outdated

collapsing if statement

677ed63

adding strict-subscriber flag

6cfb92a

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Sep 30, 2021

knative-prow-robot assigned evankanderson Sep 30, 2021

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

knative-prow-robot assigned lionelvillard Sep 30, 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 Sep 30, 2021

adding new feature flag to configmap

160df20

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 30, 2021

salaboy mentioned this pull request Sep 30, 2021

Adding strict-subscriber docs knative/docs#4311

Merged

EOF newline

38c2f65

another whitespace

8f4edda

knative-prow-robot requested review from evankanderson and lionelvillard October 13, 2021 15:37

evankanderson reviewed Oct 13, 2021

View reviewed changes

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

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

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

knative-prow-robot merged commit 288abf1 into knative:main Oct 13, 2021

creydr mentioned this pull request Nov 14, 2022

Remove strict subscriber feature flag #6608

Merged

5 tasks

Conversation

salaboy commented Sep 28, 2021

Proposed Changes

Pre-review Checklist

Uh oh!

salaboy commented Sep 28, 2021

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

salaboy commented Sep 29, 2021

Uh oh!

lionelvillard commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionelvillard commented Sep 29, 2021

Uh oh!

salaboy commented Sep 30, 2021

Uh oh!

salaboy commented Sep 30, 2021

Uh oh!

salaboy commented Sep 30, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

evankanderson commented Sep 30, 2021

Uh oh!

lionelvillard commented Sep 30, 2021

Uh oh!

lionelvillard commented Sep 30, 2021

Uh oh!

lionelvillard commented Sep 30, 2021

Uh oh!

salaboy commented Sep 30, 2021

Uh oh!

lionelvillard commented Oct 1, 2021

Uh oh!

pierDipi commented Oct 13, 2021

Uh oh!

salaboy commented Oct 13, 2021

Uh oh!

knative-metrics-robot commented Oct 13, 2021

Uh oh!

pierDipi commented Oct 13, 2021

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Oct 13, 2021

Uh oh!

pierDipi commented Oct 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lionelvillard commented Sep 29, 2021 •

edited

Loading

codecov Bot commented Sep 30, 2021 •

edited

Loading