Add support for Eventing/v1beta2 EventTypes #1829

dsimansk · 2023-07-20T11:08:21Z

Description

Changes

🎁 New Add support for Eventing/v1beta2 EventTypes

Reference

Release Note

Add support for Eventing/v1beta2 EventTypes

knative-prow · 2023-07-20T11:08:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk]

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

knative-prow

@dsimansk: 1 warning.

In response to this:

Description

Changes

🎁 New Add support for Eventing/v1beta2 EventTypes

Reference

Fixes #1818

/cc @matzew @rhuss

Release Note
Add support for Eventing/v1beta2 EventTypes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/kn/commands/flags/sink.go

rhuss · 2023-07-20T12:03:55Z

pkg/kn/commands/flags/eventtype.go

@@ -25,6 +25,6 @@ type EventtypeFlags struct {
 func (e *EventtypeFlags) Add(cmd *cobra.Command) {
 	cmd.Flags().StringVarP(&e.Type, "type", "t", "", "Cloud Event type")
 	cmd.Flags().StringVar(&e.Source, "source", "", "Cloud Event source")
-	cmd.Flags().StringVarP(&e.Broker, "broker", "b", "", "Cloud Event broker")
+	cmd.Flags().StringVarP(&e.Broker, "broker", "b", "", "Cloud Event Broker. This flag is added for the convenience, since Eventing v1beta2 brokers as represented as KReference type.")


I think this clarification is more confusing than it helps. I don't think that the user know ad hoc what a "KReference type" is. For the help message it would be better to use something like "Deprecated. Please use --reference instead", or if its not deprecated, I just would keep it as is. Anything else is just confusing,.

rhuss · 2023-07-20T12:07:30Z

pkg/kn/commands/eventtype/create.go

+		"You can specify a broker, channel, or fully qualified GVK resource. " +
+		"Examples: '" + flag + " broker:nest' for a broker 'nest', " +
+		"'" + flag + " channel:pipe' for a channel 'pipe'."
+	//TODO: craft fqdn GVK example.


Important to not forget it here, since using a GVK is the most complex way to specify this reference so there should be an example for sure.

From my e2e tests it seems there's an issue with GVK. At least it's not working for EventTypes. I'll be certainly taking a bit in-depth look. I'd say we have a bug there. :)

rhuss · 2023-07-20T12:12:16Z

Looks straightforward to me. I've added some comments to the wording.

I wonder whether we should make --broker deprecated? But I don't think we should do it, as you mentioned, it's quite convenient.

dsimansk · 2023-07-20T14:12:06Z

Looks straightforward to me. I've added some comments to the wording.

I wonder whether we should make --broker deprecated? But I don't think we should do it, as you mentioned, it's quite convenient.

I'd prefer to keep it untouched. Even deprecating will take time to remove it completely, but for the sake of convenience it's still handy for most common use case.

dsimansk · 2023-07-21T08:44:30Z

pkg/kn/commands/flags/sink.go

+		"'" + flag + " special.eventing.dev/v1alpha1/channels:pipe' for GroupVersionResource of v1alpha1 'pipe'. " +
+		"GroupVersionResource requires resource name in a lower case plural form. The inputs are sanitized on best effort basis, " +
+		"but in case of 'resource not found' error it may help to double check and correct the input. " +


@rhuss that's the case size is a culprit of my initial "bug" hunch. Can you please take a look at wording since you're master writer please?

codecov · 2023-07-21T09:06:41Z

Codecov Report

Patch coverage: 82.67% and project coverage change: -0.06 ⚠️

Comparison is base (3ecb24f) 79.73% compared to head (210841f) 79.67%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   79.73%   79.67%   -0.06%     
==========================================
  Files         179      179              
  Lines       13898    13952      +54     
==========================================
+ Hits        11081    11116      +35     
- Misses       2054     2069      +15     
- Partials      763      767       +4

Impacted Files	Coverage Δ
pkg/kn/commands/eventtype/eventtype.go	`100.00% <ø> (ø)`
pkg/kn/commands/eventtype/create.go	`76.31% <65.71%> (-11.19%)`	⬇️
pkg/kn/commands/flags/sink.go	`81.41% <68.96%> (-4.73%)`	⬇️
pkg/eventing/v1beta2/client_mock.go	`94.28% <93.75%> (ø)`
pkg/eventing/v1beta2/client.go	`85.00% <100.00%> (ø)`
pkg/kn/commands/broker/delivery_option_flags.go	`100.00% <100.00%> (ø)`
pkg/kn/commands/completion_helper.go	`85.54% <100.00%> (ø)`
pkg/kn/commands/eventtype/delete.go	`80.64% <100.00%> (ø)`
pkg/kn/commands/eventtype/describe.go	`81.25% <100.00%> (+2.30%)`	⬆️
pkg/kn/commands/eventtype/list.go	`80.26% <100.00%> (ø)`
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dsimansk · 2023-07-21T12:03:31Z

/test all

dsimansk · 2023-07-21T12:52:15Z

/retest

knative-prow · 2023-07-21T14:28:13Z

@dsimansk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
integration-tests-latest-release_client_main	`11008d1`	link	true	`/test integration-tests-latest-release`
build-tests_client_main	`210841f`	link	true	`/test build-tests`

Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dsimansk · 2023-07-21T14:48:46Z

It seems very strange that build-tests_client_main keeps failing without any meaningful error being reported. I'll close this PR and try to create a fresh one, it seems like something is out sync in Prow for some reason.

dsimansk · 2023-07-21T14:48:54Z

/close

knative-prow · 2023-07-21T14:48:57Z

@dsimansk: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow bot requested review from matzew and rhuss July 20, 2023 11:08

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2023

knative-prow bot reviewed Jul 20, 2023

View reviewed changes

pkg/kn/commands/flags/sink.go Show resolved Hide resolved

knative-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 20, 2023

rhuss reviewed Jul 20, 2023

View reviewed changes

dsimansk commented Jul 21, 2023

View reviewed changes

Add support for Eventing/v1beta2 EventTypes

d6d5617

dsimansk force-pushed the pr/eventtypes-v1beta2 branch from 11008d1 to d6d5617 Compare July 21, 2023 14:02

Debug build check

210841f

knative-prow bot closed this Jul 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Eventing/v1beta2 EventTypes #1829

Add support for Eventing/v1beta2 EventTypes #1829

dsimansk commented Jul 20, 2023

knative-prow bot commented Jul 20, 2023

knative-prow bot left a comment

rhuss Jul 20, 2023

rhuss Jul 20, 2023

dsimansk Jul 20, 2023 •

edited

Loading

rhuss commented Jul 20, 2023

dsimansk commented Jul 20, 2023

dsimansk Jul 21, 2023

codecov bot commented Jul 21, 2023 •

edited

Loading

dsimansk commented Jul 21, 2023

dsimansk commented Jul 21, 2023

knative-prow bot commented Jul 21, 2023

dsimansk commented Jul 21, 2023

dsimansk commented Jul 21, 2023

knative-prow bot commented Jul 21, 2023

Add support for Eventing/v1beta2 EventTypes #1829

Add support for Eventing/v1beta2 EventTypes #1829

Conversation

dsimansk commented Jul 20, 2023

Description

Changes

Reference

knative-prow bot commented Jul 20, 2023

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

rhuss Jul 20, 2023

Choose a reason for hiding this comment

rhuss Jul 20, 2023

Choose a reason for hiding this comment

dsimansk Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

rhuss commented Jul 20, 2023

dsimansk commented Jul 20, 2023

dsimansk Jul 21, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

dsimansk commented Jul 21, 2023

dsimansk commented Jul 21, 2023

knative-prow bot commented Jul 21, 2023

dsimansk commented Jul 21, 2023

dsimansk commented Jul 21, 2023

knative-prow bot commented Jul 21, 2023

dsimansk Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading