Removes the deprecated DeadLetterChannel in ChannelableStatus #6722

vishal-chdhry · 2023-02-04T04:35:37Z

Signed-off-by: Vishal Choudhary sendtovishalchoudhary@gmail.com

Proposed Changes

🗑️ Remove feature or internal logic
removes deprecated DeadLetterChannel https://github.com/knative/eventing/blob/main/pkg/apis/duck/v1/channelable_types.go#L70-L74

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

removes deprecated DeadLetterChannel in favor of DeliveryStatus

Docs

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

knative-prow · 2023-02-04T04:35:47Z

Hi @vishal-chdhry. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vishal-chdhry · 2023-02-04T04:43:35Z

@aliok I had the following doubts

There are a few references of DeadLetterChannel in v1beta1 as well

eventing/pkg/apis/duck/v1beta1/delivery_types.go

Lines 113 to 117 in 8b7551c

    
           type DeliveryStatus struct { 
        
           	// DeadLetterChannel is a KReference that is the reference to the native, platform specific channel 
        
           	// where failed events are sent to. 
        
           	// +optional 
        
           	DeadLetterChannel *duckv1.KReference `json:"deadLetterChannel,omitempty"`

eventing/pkg/apis/duck/v1beta1/channelable_types.go

Lines 63 to 66 in 8b7551c

    
           // DeadLetterChannel is a KReference and is set by the channel when it supports native error handling via a channel 
        
           // Failed messages are delivered here. 
        
           // +optional 
        
           DeadLetterChannel *duckv1.KReference `json:"deadLetterChannel,omitempty"`

but these two are not deprecated, should i remove them too? I have verified that these two are not in use.

How do I send the emails to knative users email list?
What is the command for codegen, i could not find the makefile

vishal-chdhry · 2023-02-04T04:45:51Z

/hold

aliok · 2023-02-04T18:56:47Z

@vishal-chdhry

There are a few references of DeadLetterChannel in v1beta1 as well

No, the ones in v1beta1 need to stay.

How do I send the emails to knative users email list?

Here's the link to the Google Group: https://groups.google.com/g/knative-users
There is also knative-dev: https://groups.google.com/g/knative-dev

I think sending an email to both makes more sense.

What is the command for codegen, i could not find the makefile

You can run ./hack/update-codegen.sh. That will regenerate the zz_generated.deepcopy.go file relevant here.

aliok · 2023-02-04T18:57:42Z

/assign

Assigning myself as a reviewer

aliok · 2023-02-04T18:58:06Z

/ok-to-test

aliok · 2023-02-04T19:00:27Z

Ah, one more thing. Can you also fill the Release Note section in the PR description that this field is removed? It would be nice to note that in the release notes. For example, #6665 has a release note that is shown in the 1.9.2 release

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

codecov · 2023-02-04T20:18:59Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (8b7551c) 80.53% compared to head (936956c) 80.53%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6722   +/-   ##
=======================================
  Coverage   80.53%   80.53%           
=======================================
  Files         236      236           
  Lines       12100    12100           
=======================================
  Hits         9745     9745           
  Misses       1867     1867           
  Partials      488      488

Impacted Files	Coverage Δ
pkg/apis/duck/v1/channelable_types.go	`96.61% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

aliok · 2023-02-06T09:19:42Z

There are some failures in some Github actions.

Downstream / Unit Test (eventing-natss, knative-sandbox): We should simply create a ticket in https://github.com/knative-sandbox/eventing-natss, referencing this PR and the ticket
KinD e2e tests / e2e tests (v1.25.3, ./test/experimental): Needs to be checked
KinD e2e tests / e2e tests (v1.25.3, ./test/e2e): Needs to be checked
KinD e2e tests / e2e tests (v1.25.3, ./test/rekt/...): Needs to be checked

Let's first do the sanity check for the last 3. Are they passing on other PRs? Or, are they really getting broken by this PR?

vishal-chdhry · 2023-02-06T09:50:24Z

we have failing tests in this PR as well #6696

aliok · 2023-02-06T13:13:01Z

Are they failing the same way?
Also KinD e2e tests / e2e tests (v1.25.3, ./test/e2e) is not failing there. I am triggering these again now

vishal-chdhry · 2023-02-06T14:19:53Z

@aliok Yes both have the same error

runtime.main
	/opt/hostedtoolcache/go/1.18.10/x64/src/runtime/proc.go:250
--- PASS: ExampleWithRetry (0.00s)
PASS
ok  	knative.dev/eventing/test/rekt/resources/trigger	0.271s
FAIL
Error: Process completed with exit code 1.

aliok · 2023-02-07T07:10:11Z

ok, then we're all good. Can you unhold after a week from your email you sent to the community?

aliok · 2023-02-07T07:10:18Z

/lgtm
/approve

knative-prow · 2023-02-07T07:10:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, Vishal-Chdhry

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

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

matzew · 2023-02-07T08:20:16Z

I am OK w/ this direct removal, since the API has been deprecated in 2021, and was meant to be removed. So we are OK in NOT bumping to v2alpha1 ... 😄

Usually we should do the migration etc.

Note: NATSS downstream has issues, we just ignore this.

matzew · 2023-02-07T08:21:57Z

Thanks for the contribution, @vishal-chdhry

vishal-chdhry · 2023-02-07T09:10:19Z

@matzew I have opened an issue in natss regarding this knative-extensions/eventing-natss#388.
Should I go ahead and remove the references to DeadLetterChannel there as well??

pierDipi · 2023-02-08T11:20:33Z

/cc aliok
/uncc

aliok · 2023-02-08T13:21:02Z

Should I go ahead and remove the references to DeadLetterChannel there as well??

@vishal-chdhry I don't know the latest status of Nats repository. If you think you can fix the issue there, feel free and give it a try.

aliok · 2023-03-02T07:31:20Z

/unhold

…e#6722) Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com> Fixes knative#6720  ## Proposed Changes  - 🗑️ Remove feature or internal logic - removes deprecated `DeadLetterChannel` https://github.com/knative/eventing/blob/main/pkg/apis/duck/v1/channelable_types.go#L70-L74 ### Pre-review Checklist  - [x] **At least 80% unit test coverage** - [x] **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 removes deprecated DeadLetterChannel in favor of DeliveryStatus ``` **Docs**  --------- Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

removes deadletterchannel

0df31b1

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 4, 2023

knative-prow bot requested review from odacremolbap and pierDipi February 4, 2023 04:35

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2023

vishal-chdhry changed the title ~~Removes the deprecated DeadLetterChannel in ChannelableStatus #6720~~ Removes the deprecated DeadLetterChannel in ChannelableStatus Feb 4, 2023

knative-prow bot assigned aliok Feb 4, 2023

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 4, 2023

codegen

936956c

Signed-off-by: Vishal Choudhary <sendtovishalchoudhary@gmail.com>

knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 4, 2023

vishal-chdhry mentioned this pull request Feb 6, 2023

Get rid of the removed DeadLetterChannel in ChannelableStatus knative-extensions/eventing-natss#388

Closed

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2023

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

knative-prow bot requested review from aliok and removed request for pierDipi February 8, 2023 11:20

vishal-chdhry mentioned this pull request Feb 9, 2023

Removes the deprecated DeadLetterChannel in ChannelableStatus knative-extensions/eventing-natss#389

Closed

5 tasks

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2023

knative-prow bot merged commit 7d7df2d into knative:main Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes the deprecated DeadLetterChannel in ChannelableStatus #6722

Removes the deprecated DeadLetterChannel in ChannelableStatus #6722

vishal-chdhry commented Feb 4, 2023 •

edited

Loading

knative-prow bot commented Feb 4, 2023

vishal-chdhry commented Feb 4, 2023 •

edited

Loading

vishal-chdhry commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

codecov bot commented Feb 4, 2023 •

edited

Loading

aliok commented Feb 6, 2023

vishal-chdhry commented Feb 6, 2023

aliok commented Feb 6, 2023

vishal-chdhry commented Feb 6, 2023

aliok commented Feb 7, 2023

aliok commented Feb 7, 2023

knative-prow bot commented Feb 7, 2023

matzew commented Feb 7, 2023

matzew commented Feb 7, 2023

vishal-chdhry commented Feb 7, 2023 •

edited

Loading

pierDipi commented Feb 8, 2023

aliok commented Feb 8, 2023

aliok commented Mar 2, 2023

Removes the deprecated DeadLetterChannel in ChannelableStatus #6722

Removes the deprecated DeadLetterChannel in ChannelableStatus #6722

Conversation

vishal-chdhry commented Feb 4, 2023 • edited Loading

Proposed Changes

Pre-review Checklist

knative-prow bot commented Feb 4, 2023

vishal-chdhry commented Feb 4, 2023 • edited Loading

vishal-chdhry commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

aliok commented Feb 4, 2023

codecov bot commented Feb 4, 2023 • edited Loading

Codecov Report

aliok commented Feb 6, 2023

vishal-chdhry commented Feb 6, 2023

aliok commented Feb 6, 2023

vishal-chdhry commented Feb 6, 2023

aliok commented Feb 7, 2023

aliok commented Feb 7, 2023

knative-prow bot commented Feb 7, 2023

matzew commented Feb 7, 2023

matzew commented Feb 7, 2023

vishal-chdhry commented Feb 7, 2023 • edited Loading

pierDipi commented Feb 8, 2023

aliok commented Feb 8, 2023

aliok commented Mar 2, 2023

vishal-chdhry commented Feb 4, 2023 •

edited

Loading

vishal-chdhry commented Feb 4, 2023 •

edited

Loading

codecov bot commented Feb 4, 2023 •

edited

Loading

vishal-chdhry commented Feb 7, 2023 •

edited

Loading