Implement `KReference.Group` resolution for `Subscriber.Ref`#5440

slinkydeveloper · 2021-05-27T09:55:09Z

Part of the experimental feature #5086
Requires knative/pkg#2127

Proposed Changes

🎁 Add experimental feature kreference-group
🎁 Implement KReference.Group resolution for Subscriber.Ref
🎁 Included code to enable reading the experimental config map in the eventing-webhook (I can move these to another PR if you want to)

Pre-review Checklist

At least 80% unit test coverage: All the additional code is unit tested
E2E tests for any new behavior: Added a channel to channel e2e test
Docs PR for any user-facing impact Documentation for KReference.Group feature docs#3713
Spec PR for any new API feature: N/A (ALPHA feature)
Conformance test for any change to the spec: N/A (ALPHA feature)

Release Note

Added the experimental feature kreference-group. Enabling it, you can use Subscriber.Ref.Group instead of Subscriber.Ref.APIVersion to refer to another Resource, without being explicit about the resource version (e.g. v1beta1, v1, ...)

slinkydeveloper · 2021-05-27T09:56:12Z

/hold

This requires the lazy consensus approval per https://github.com/knative/eventing/blob/main/docs/experimental-features.md#feature-proposal-development-and-graduation-process

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

* Added tristate flags Renamed experimental features config map Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Update doc Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * goimports Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Added some nits from #5440 Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Boilerplate Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Fixed bad links in the doc and bad name Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

cmd/webhook/main.go

config/core/configmaps/features.yaml

config/core/resources/subscription.yaml

go.mod

pkg/apis/feature/flag_names.go

pkg/reconciler/testing/v1/subscription.go

n3wscott · 2021-06-07T21:22:58Z

test/experimental/features/kreference_group/channel_to_channel.go

+	channelGroup := channelGVK.GroupKind().Group
+	channelAPIVersion, imcKind := channelGVK.ToAPIVersionAndKind()
+
+	channelAName := feature.MakeRandomK8sName("channel-a")


This code is much more clear if you use the prober code. See the broker/channel conformance tests for an example.

n3wscott · 2021-06-07T21:23:35Z

test/experimental/features/kreference_group/channel_to_channel.go

+
+	f.Setup("Install channel A -> channel B subscription", func(ctx context.Context, t feature.T) {
+		namespace := environment.FromContext(ctx).Namespace()
+		_, err := eventingclient.Get(ctx).MessagingV1().Subscriptions(namespace).Create(ctx,


Rather than do this, you can use the subscription.Install resource package?

TBH I think this code is cleaner. Also I was worried to use it, because in order to do so, I need to modify the template to allow for the new group field. I would rather keep it this way and revisit in future.

test/experimental/main_test.go

n3wscott · 2021-06-07T21:26:57Z

test/experimental/main_test.go

@@ -24,7 +24,7 @@ import (
 	"testing"


This file needs a build flag set on it, maybe +build e2e,experimental

Do we need to have another flag, given we already have e2e? Isn't that enough? I see we don't make difference between e2e/conformance/rekt in terms of build flags, they all use e2e

slinkydeveloper · 2021-06-08T06:43:38Z

Depends on #5479

slinkydeveloper · 2021-06-15T09:37:05Z

pkg PR is merged, I've added the new changes here but I probably need to let #5495 go in before updating this one deps

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

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

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

knative-metrics-robot · 2021-06-15T13:27:31Z

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/feature/store.go	88.2%	85.0%	-3.2
pkg/reconciler/subscription/subscription.go	86.8%	85.9%	-0.9

codecov · 2021-06-15T13:29:17Z

Codecov Report

Merging #5440 (46afa33) into main (8a110a1) will decrease coverage by 0.07%.
The diff coverage is 61.90%.

@@            Coverage Diff             @@
##             main    #5440      +/-   ##
==========================================
- Coverage   82.69%   82.62%   -0.08%     
==========================================
  Files         198      198              
  Lines        6098     6117      +19     
==========================================
+ Hits         5043     5054      +11     
- Misses        731      737       +6     
- Partials      324      326       +2

Impacted Files	Coverage Δ
pkg/reconciler/subscription/subscription.go	`80.40% <40.00%> (-1.72%)`	⬇️
pkg/apis/feature/store.go	`81.48% <50.00%> (-6.02%)`	⬇️
pkg/reconciler/subscription/controller.go	`100.00% <100.00%> (ø)`

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 8a110a1...46afa33. Read the comment docs.

slinkydeveloper · 2021-06-15T13:43:48Z

/unhold

This is now ready to be merged

matzew

Thanks for this

/approve

knative-prow-robot · 2021-06-16T09:32:33Z

[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

slinkydeveloper · 2021-06-16T09:36:40Z

/hold
let's let somebody else have a look before merging :)

config/core/resources/subscription.yaml

lionelvillard · 2021-06-16T15:23:08Z

config/core/configmaps/features.yaml

 data:
+  # ALPHA feature: The kreference-group allows you to use the Group field in KReferences.
+  # For more details: https://github.com/knative/eventing/issues/5086
+  kreference-group: "disabled"


It's probably fine for now but eventually it should look more like https://github.com/knative/serving/blob/main/config/core/configmaps/features.yaml.

Mhh yeah seems like we don't have the example, but do we really need it? Isn't simpler for the user to have the flags already configured? Also, considering at some point some of them will become enabled by default?

lionelvillard · 2021-06-16T15:29:38Z

This is great, thanks for doing this!

/lgtm

slinkydeveloper · 2021-06-16T15:31:20Z

/unhold

slinkydeveloper · 2021-06-16T16:23:37Z

/retest

…native#5440)" This reverts commit 6adafe5

slinkydeveloper force-pushed the kreference_group branch from c5bcca8 to e1224b2 Compare May 27, 2021 09:55

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 27, 2021

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

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 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 May 27, 2021

This was referenced May 27, 2021

[Experimental] KReference.Group to avoid being explicit on the APIVersion #5086

Open

Subscription Channel ref without api version #5131

Closed

slinkydeveloper added a commit to slinkydeveloper/eventing that referenced this pull request Jun 1, 2021

Added some nits from knative#5440

36a744d

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

slinkydeveloper force-pushed the kreference_group branch from e1224b2 to 62761f8 Compare June 1, 2021 09:17

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

slinkydeveloper mentioned this pull request Jun 3, 2021

Documentation for KReference.Group feature knative/docs#3713

Merged

n3wscott reviewed Jun 7, 2021

View reviewed changes

slinkydeveloper mentioned this pull request Jun 8, 2021

Modify experimental features e2e test setup #5479

Merged

5 tasks

slinkydeveloper force-pushed the kreference_group branch from c677412 to 6772bb3 Compare June 9, 2021 10:17

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

slinkydeveloper requested a review from n3wscott June 10, 2021 16:13

slinkydeveloper force-pushed the kreference_group branch from 322132f to 7f2cc7b Compare June 15, 2021 09:35

slinkydeveloper mentioned this pull request Jun 15, 2021

Add context wrappers to genreconciler knative/pkg#2147

Closed

slinkydeveloper added 3 commits June 15, 2021 14:27

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

420c55b

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

Removed custom build tag and using just e2e tag

a610b08

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

Progress

aa4fc79

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

slinkydeveloper added 4 commits June 15, 2021 14:27

Newline

d984d6d

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

Remove hacky replace

89b8da6

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

Updated with latest changes

18ddec0

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

Added knative_reference

539d817

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

slinkydeveloper force-pushed the kreference_group branch from 7f2cc7b to 539d817 Compare June 15, 2021 12:43

slinkydeveloper added 3 commits June 15, 2021 15:02

Fixed UT

8fd4096

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

Missing newline

dffd618

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

Fixed UT

46afa33

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 15, 2021

matzew approved these changes Jun 16, 2021

View reviewed changes

knative-prow-robot assigned matzew Jun 16, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 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 Jun 16, 2021

lionelvillard reviewed Jun 16, 2021

View reviewed changes

config/core/resources/subscription.yaml Show resolved Hide resolved

lionelvillard reviewed Jun 16, 2021

View reviewed changes

knative-prow-robot assigned lionelvillard Jun 16, 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 Jun 16, 2021

knative-prow-robot merged commit 6adafe5 into knative:main Jun 16, 2021

slinkydeveloper deleted the kreference_group branch June 16, 2021 16:50

aliok added a commit to aliok/eventing that referenced this pull request Jun 21, 2021

Revert "Implement KReference.Group resolution for Subscriber.Ref (k…

29be049

…native#5440)" This reverts commit 6adafe5

slinkydeveloper mentioned this pull request Jul 5, 2021

Now URIResolver can resolve KReference.Group knative/pkg#2179

Closed

lionelvillard mentioned this pull request Sep 29, 2021

Updating validation tests to follow the spec for spec.subscriber #5762

Merged

5 tasks

Conversation

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

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

n3wscott Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

n3wscott Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper commented Jun 8, 2021

Uh oh!

slinkydeveloper commented Jun 15, 2021

Uh oh!

knative-metrics-robot commented Jun 15, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

slinkydeveloper commented Jun 15, 2021

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jun 16, 2021

Uh oh!

slinkydeveloper commented Jun 16, 2021

Uh oh!

Uh oh!

lionelvillard Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

lionelvillard commented Jun 16, 2021

Uh oh!

slinkydeveloper commented Jun 16, 2021

Uh oh!

slinkydeveloper commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

slinkydeveloper commented May 27, 2021 •

edited

Loading

slinkydeveloper commented May 27, 2021 •

edited

Loading

codecov bot commented Jun 15, 2021 •

edited

Loading