Remove deprecated httpclient msgsender #7018

g1rjeevan · 2023-06-13T14:01:09Z

Proposed Changes

Removed the legacy http client go, message sender go and respective test files, Since the functionality are handled in http client new and message sender new go

Issue related : #6995

🗑️ Remove feature or internal logic

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

Docs

📖 #6995

knative-prow · 2023-06-13T14:01:21Z

Hi @g1rjeevan. 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.

Cali0707 · 2023-06-13T17:59:50Z

cmd/heartbeats/main.go

@@ -222,7 +222,10 @@ func main() {

 // maybeQuitIstioProxy shuts down Istio's proxy when available.
 func maybeQuitIstioProxy() {
-	_, err := http.DefaultClient.Get("http://localhost:15020/quitquitquit")
+	req, _ := http.NewRequest(http.MethodPost, "http://localhost:15020/quitquitquit", nil)


Is there a reason why we are switching from making a GET request to making a POST request here?

Actually, This is the code after taking merge from main, It must be already in the main branch since it mentioned as outdated. Can you confirm once?

Good point, I was looking at an outdated diff - sorry for the confusion!

Cali0707 · 2023-06-13T18:00:55Z

test/test_images/wathola-fetcher/main.go

@@ -31,7 +31,10 @@ func main() {
 }

 func maybeQuitIstioProxy() {
-	_, err := http.DefaultClient.Get("http://localhost:15020/quitquitquit")
+	req, _ := http.NewRequest(http.MethodPost, "http://localhost:15020/quitquitquit", nil)


Same here, should this also be a GET request still?

Same here as the other one.

Cali0707 · 2023-06-13T18:03:25Z

Thanks for the contribution @g1rjeevan! Welcome! 🎉

Cali0707 · 2023-06-14T11:13:56Z

/ok-to-test

Cali0707 · 2023-06-15T14:16:57Z

Hey @g1rjeevan it looks like the ingress handler and some of the tests need to be updated to reflect removing the httpclient msgsender:

pkg/broker/ingress/ingress_handler.go:56:24: undefined: kncloudevents.HTTPMessageSender
pkg/inmemorychannel/message_dispatcher_benchmark_test.go:80:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/inmemorychannel/message_dispatcher_test.go:262:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/kncloudevents/http_client_new_test.go:91:23: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:113:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:114:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:124:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:125:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:141:79: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:142:86: undefined: castToTransport

g1rjeevan · 2023-06-21T03:52:49Z

Hey @g1rjeevan it looks like the ingress handler and some of the tests need to be updated to reflect removing the httpclient msgsender:

pkg/broker/ingress/ingress_handler.go:56:24: undefined: kncloudevents.HTTPMessageSender
pkg/inmemorychannel/message_dispatcher_benchmark_test.go:80:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/inmemorychannel/message_dispatcher_test.go:262:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/kncloudevents/http_client_new_test.go:91:23: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:113:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:114:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:124:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:125:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:141:79: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:142:86: undefined: castToTransport

g1rjeevan · 2023-06-21T03:57:41Z

Hey @g1rjeevan it looks like the ingress handler and some of the tests need to be updated to reflect removing the httpclient msgsender:

pkg/broker/ingress/ingress_handler.go:56:24: undefined: kncloudevents.HTTPMessageSender
pkg/inmemorychannel/message_dispatcher_benchmark_test.go:80:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/inmemorychannel/message_dispatcher_test.go:262:35: undefined: kncloudevents.NewHTTPMessageSenderWithTarget
pkg/kncloudevents/http_client_new_test.go:91:23: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:113:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:114:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:124:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:125:25: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:141:79: undefined: castToTransport
pkg/kncloudevents/http_client_new_test.go:142:86: undefined: castToTransport

In order to change these tests cases, there are pre-tasks that need to be completed.
Pre-Tasks:

creydr · 2023-07-10T08:21:06Z

Hey @g1rjeevan,
the mt-broker-ingress component migrated to the new package too. So there should be no usages of the old HTTMessagSender left (besides of one unit test (pkg/inmemorychannel/message_dispatcher_test.go). Can you rebase your PR to get the latest changes and remove the last usage in the unit test?

eventing/pkg/inmemorychannel/message_dispatcher_test.go

Lines 262 to 275 in a4f0c10

    
           httpsender, err := kncloudevents.NewHTTPMessageSenderWithTarget(channelAProxy.URL) 
        
           if err != nil { 
        
           	t.Fatal(err) 
        
           } 
        
           req, err := httpsender.NewCloudEventRequest(context.Background()) 
        
           if err != nil { 
        
           	t.Fatal(err) 
        
           } 
        
           event := test.FullEvent() 
        
           _ = protocolhttp.WriteRequest(context.Background(), binding.ToMessage(&event), req) 
        
           res, err := httpsender.Send(req)

g1rjeevan · 2023-07-13T06:13:38Z

Hey @g1rjeevan, the mt-broker-ingress component migrated to the new package too. So there should be no usages of the old HTTMessagSender left (besides of one unit test (pkg/inmemorychannel/message_dispatcher_test.go). Can you rebase your PR to get the latest changes and remove the last usage in the unit test?

eventing/pkg/inmemorychannel/message_dispatcher_test.go

Lines 262 to 275 in a4f0c10

httpsender, err := kncloudevents.NewHTTPMessageSenderWithTarget(channelAProxy.URL)

if err != nil {

t.Fatal(err)

}

req, err := httpsender.NewCloudEventRequest(context.Background())

if err != nil {

t.Fatal(err)

}

event := test.FullEvent()

_ = protocolhttp.WriteRequest(context.Background(), binding.ToMessage(&event), req)

res, err := httpsender.Send(req)

Sure. Will push the update.

linux-foundation-easycla · 2023-07-13T08:45:45Z

✅ login: g1rjeevan / name: Jeevan (2a88d67, 5931f45, fd39d49, d7391ce, 59efdbb, 5e1a422)
❌ The email address for the commit (07bbc7e) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla · 2023-07-13T08:50:11Z

✅ login: g1rjeevan / name: Jeevan (2a88d67, 5931f45, fd39d49, d7391ce, 59efdbb, 5e1a422, 4d78544)
❌ The email address for the commit (07bbc7e, 50960df) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla · 2023-07-13T08:58:01Z

✅ login: g1rjeevan / name: Jeevan (2a88d67, 5931f45, fd39d49, d7391ce, 59efdbb, 5e1a422, 4d78544, 412a6cd)
❌ The email address for the commit (07bbc7e, 50960df, 4a60121) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla · 2023-07-13T09:18:46Z

✅ login: g1rjeevan / name: Jeevan (2a88d67, 5931f45, fd39d49, d7391ce, 59efdbb, 5e1a422, 4d78544, 412a6cd, 229dfd7, 2d23659)
❌ The email address for the commit (07bbc7e, 50960df, 4a60121) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

Removed message sender usage from message dispatcher test Removed message sender usage from message dispatcher test

vendor/golang.org/x/sys/unix/syscall_unix.go

pkg/inmemorychannel/message_dispatcher_benchmark_test.go

Cali0707

Looks like the tests are now passing!
/lgtm
/cc @creydr

g1rjeevan · 2023-07-21T02:40:43Z

/cc @creydr @Cali0707 @lberk

g1rjeevan · 2023-07-21T02:54:52Z

/assign @g1rjeevan

codecov · 2023-07-21T07:11:49Z

Codecov Report

Patch coverage has no change and project coverage change: -0.13% ⚠️

Comparison is base (6911db0) 78.34% compared to head (54abccc) 78.21%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7018      +/-   ##
==========================================
- Coverage   78.34%   78.21%   -0.13%     
==========================================
  Files         250      248       -2     
  Lines       13320    13245      -75     
==========================================
- Hits        10435    10360      -75     
  Misses       2355     2355              
  Partials      530      530

Files Changed	Coverage Δ
pkg/kncloudevents/http_client_new.go	`60.33% <ø> (-0.33%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

creydr

Besides of the comments I have, I am wondering if we should wait with merging this for after the release cut (in a few days), just to make sure if any of the subprojects is using this, they have time with fixing it (IIRC rabbitmq-broker was using it).

pkg/inmemorychannel/message_dispatcher_benchmark_test.go

Cali0707 · 2023-07-21T14:06:46Z

Btw @g1rjeevan unless there are merge conflicts between this branch and the main branch, you don't need to worry about merging main into your branch - the Prow bot will take care of it when it goes to merge the PR

creydr

@g1rjeevan Thanks for your work so far 👍
Could you fix the lint errors in pkg/kncloudevents/http_client_new_test.go too? You can see the issues, when you're on the "Files changed" tab on your PR and then check on the annotations.

creydr

Thanks a lot for your work @g1rjeevan and your patience!
/lgtm

knative-prow · 2023-07-27T06:37:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, creydr, g1rjeevan

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

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

creydr · 2023-07-27T07:01:03Z

/retest

g1rjeevan · 2023-07-29T05:28:54Z

Thanks a lot for your work @g1rjeevan and your patience! /lgtm

@creydr @Cali0707 @lberk Thank you for your assistance in the contribution, and I am eager to engage further with Knative.

creydr · 2023-07-31T06:34:31Z

@creydr @Cali0707 @lberk Thank you for your assistance in the contribution, and I am eager to engage further with Knative.

That is great to hear @g1rjeevan 🎉 Most of our projects have issues labeled with good-first-issue or kind/good-first-issue (e.g. in eventing) and the "community" section on our website (https://knative.dev/docs/community/) are good starts for new contributors.
Also feel free (as you've done already 😃) to reach out to us on Slack or in one of our Workgroup meetings.
Welcome 👋 😃

## Proposed Changes Removed the legacy http client go, message sender go and respective test files, Since the functionality are handled in http client new and message sender new go Issue related : knative#6995 - 🗑️ Remove feature or internal logic - - - ### Pre-review Checklist - [x] **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 ``` **Docs** 📖 knative#6995 --------- Co-authored-by: Christoph Stäbler <[email protected]>

* Remove deprecated httpclient msgsender (knative#7018) ## Proposed Changes Removed the legacy http client go, message sender go and respective test files, Since the functionality are handled in http client new and message sender new go Issue related : knative#6995 - 🗑️ Remove feature or internal logic - - - ### Pre-review Checklist - [x] **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 ``` **Docs** 📖 knative#6995 --------- Co-authored-by: Christoph Stäbler <[email protected]> * Refactor kncloudevents and add `SendEvent` function (knative#7092) Adds a function (`SendEvent`) to the kncloudevents package to Send events. This function: * configures the client (e.g. for TLS) * has option to send replies * has option to send to a DLS * has option to add additional transformers This allows e.g. for use cases like the following: ``` kncloudevents.SendEvent(ctx, event, sub.Subscriber, kncloudevents.WithHeader(additionalHeaders), kncloudevents.WithReply(sub.Reply), kncloudevents.WithDeadLetterSink(sub.DeadLetter), kncloudevents.WithRetryConfig(sub.RetryConfig)) ``` The `SendMessage` function is provided to make integration with existing packages easier (e.g. in dependent projects). Also migrated the usages of kncloudevents.NewCloudEventRequest() to the new SendEvent() function (mt-broker-filter and mt-broker-ingress): * 74c1552 * 9728713 * 958722d * Remove deprecated kncloudevents.CloudEventsRequest (knative#7146) * Remove kncloudevents.CloudEventsRequest * Run hack/update-codegen.sh * Add unit tests for generateBackoffFn() * Fix event dispatcher library data race (knative#7280) Fix dispatcher data race Signed-off-by: Pierangelo Di Pilato <[email protected]> --------- Signed-off-by: Pierangelo Di Pilato <[email protected]> Co-authored-by: Jeevan <[email protected]> Co-authored-by: Christoph Stäbler <[email protected]>

g1rjeevan added 2 commits June 13, 2023 19:26

remove legacy http client and message sender

2a88d67

Merge branch 'main' into remove-deprecated-httpclient-msgsender

5931f45

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 13, 2023

knative-prow bot requested review from creydr and lberk June 13, 2023 14:01

Cali0707 reviewed Jun 13, 2023

View reviewed changes

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 Jun 14, 2023

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

fd39d49

g1rjeevan closed this Jun 21, 2023

g1rjeevan reopened this Jun 21, 2023

g1rjeevan added 2 commits June 21, 2023 09:28

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

d7391ce

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

59efdbb

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

5e1a422

Removed legacy http client and message sender along with usage.

a2b7256

Removed message sender usage from message dispatcher test Removed message sender usage from message dispatcher test

g1rjeevan force-pushed the remove-deprecated-httpclient-msgsender branch from 2d23659 to a2b7256 Compare July 13, 2023 09:43

creydr reviewed Jul 13, 2023

View reviewed changes

vendor/golang.org/x/sys/unix/syscall_unix.go Outdated Show resolved Hide resolved

pkg/inmemorychannel/message_dispatcher_benchmark_test.go Show resolved Hide resolved

Cali0707 approved these changes Jul 20, 2023

View reviewed changes

knative-prow bot requested a review from creydr July 20, 2023 14:52

knative-prow bot assigned Cali0707 Jul 20, 2023

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

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

26efabb

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2023

knative-prow bot requested a review from Cali0707 July 21, 2023 02:40

knative-prow bot assigned g1rjeevan Jul 21, 2023

creydr mentioned this pull request Jul 21, 2023

Remove deprecated HTTPMessageSender usage knative-extensions/eventing-rabbitmq#1176

Merged

creydr reviewed Jul 21, 2023

View reviewed changes

pkg/inmemorychannel/message_dispatcher_benchmark_test.go Outdated Show resolved Hide resolved

Merge branch 'knative:main' into remove-deprecated-httpclient-msgsender

67e22e1

remove the dispatcher benchmark test

54b3be8

creydr reviewed Jul 24, 2023

View reviewed changes

import fix

54abccc

creydr approved these changes Jul 27, 2023

View reviewed changes

knative-prow bot assigned creydr Jul 27, 2023

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

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

knative-prow bot merged commit ffa5915 into knative:main Jul 27, 2023
25 of 28 checks passed

creydr mentioned this pull request Jul 31, 2023

Remove old kncloudevents.MessageSender #6995

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated httpclient msgsender #7018

Remove deprecated httpclient msgsender #7018

g1rjeevan commented Jun 13, 2023 •

edited

Loading

knative-prow bot commented Jun 13, 2023

Cali0707 Jun 13, 2023

g1rjeevan Jun 15, 2023 •

edited

Loading

Cali0707 Jun 15, 2023

Cali0707 Jun 13, 2023

g1rjeevan Jun 15, 2023

Cali0707 commented Jun 13, 2023

Cali0707 commented Jun 14, 2023

Cali0707 commented Jun 15, 2023

g1rjeevan commented Jun 21, 2023

g1rjeevan commented Jun 21, 2023

creydr commented Jul 10, 2023

g1rjeevan commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

Cali0707 left a comment

g1rjeevan commented Jul 21, 2023

g1rjeevan commented Jul 21, 2023

codecov bot commented Jul 21, 2023 •

edited

Loading

creydr left a comment

Cali0707 commented Jul 21, 2023

creydr left a comment

creydr left a comment

knative-prow bot commented Jul 27, 2023

creydr commented Jul 27, 2023

g1rjeevan commented Jul 29, 2023

creydr commented Jul 31, 2023

Remove deprecated httpclient msgsender #7018

Remove deprecated httpclient msgsender #7018

Conversation

g1rjeevan commented Jun 13, 2023 • edited Loading

Proposed Changes

Pre-review Checklist

knative-prow bot commented Jun 13, 2023

Cali0707 Jun 13, 2023

Choose a reason for hiding this comment

g1rjeevan Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Cali0707 Jun 15, 2023

Choose a reason for hiding this comment

Cali0707 Jun 13, 2023

Choose a reason for hiding this comment

g1rjeevan Jun 15, 2023

Choose a reason for hiding this comment

Cali0707 commented Jun 13, 2023

Cali0707 commented Jun 14, 2023

Cali0707 commented Jun 15, 2023

g1rjeevan commented Jun 21, 2023

g1rjeevan commented Jun 21, 2023

creydr commented Jul 10, 2023

g1rjeevan commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

linux-foundation-easycla bot commented Jul 13, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

g1rjeevan commented Jul 21, 2023

g1rjeevan commented Jul 21, 2023

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

creydr left a comment

Choose a reason for hiding this comment

Cali0707 commented Jul 21, 2023

creydr left a comment

Choose a reason for hiding this comment

creydr left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jul 27, 2023

creydr commented Jul 27, 2023

g1rjeevan commented Jul 29, 2023

creydr commented Jul 31, 2023

g1rjeevan commented Jun 13, 2023 •

edited

Loading

g1rjeevan Jun 15, 2023 •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading