Skip to content

Use the Request in Forwarder directly#575

Merged
knative-prow[bot] merged 1 commit intoknative-extensions:mainfrom
mgencur:forward_directly
Aug 10, 2023
Merged

Use the Request in Forwarder directly#575
knative-prow[bot] merged 1 commit intoknative-extensions:mainfrom
mgencur:forward_directly

Conversation

@mgencur
Copy link
Copy Markdown
Contributor

@mgencur mgencur commented Aug 9, 2023 •
edited
Loading

This is a follow-up to #572 . This PR doesn't choose specific headers but forwards everything by re-using the original request and modifying it.

Changes

/kind

Fixes #

Release Note

Docs

@knative-prow
Copy link
Copy Markdown

knative-prow bot commented Aug 9, 2023

@mgencur: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

Details

In response to this:

Changes

/kind

Fixes #

Release Note

Docs

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 knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023
@knative-prow knative-prow bot requested review from psschwei and upodroid August 9, 2023 13:20
@knative-prow knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2023
@mgencur mgencur mentioned this pull request Aug 9, 2023
Merged
ReToCode
ReToCode approved these changes Aug 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ReToCode ReToCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks even better, thanks @mgencur

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
@mgencur mgencur changed the title [WIP] Use the Request in Forwarder directly Use the Request in Forwarder directly Aug 10, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2023
matzew
matzew reviewed Aug 10, 2023
View reviewed changes
pkg/eventshub/forwarder/forwarder.go
}

req.Header = headersToBeSent
req := request.Clone(requestCtx)
Copy link
Copy Markdown
Contributor

@matzew matzew Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this clone approach

matzew
matzew approved these changes Aug 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@matzew matzew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@knative-prow
Copy link
Copy Markdown

knative-prow bot commented Aug 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, mgencur, ReToCode

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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2023
@knative-prow knative-prow bot merged commit a7237b0 into knative-extensions:main Aug 10, 2023
@ReToCode
Copy link
Copy Markdown
Contributor

ReToCode commented Aug 10, 2023

@mgencur cherry-pick to 1.10 and 1.11 or go with what we have there?

@mgencur
Copy link
Copy Markdown
Contributor Author

mgencur commented Aug 10, 2023

I would not bother but we can cherry-pick. I'm fine with both.

creydr added a commit to creydr/knative-reconciler-test that referenced this pull request Feb 28, 2024
@creydr
Revert "Use the Request in Forwarder directly (knative-extensions#575)"
8cbf4a1 
This reverts commit a7237b0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matzew matzew matzew approved these changes

@psschwei psschwei Awaiting requested review from psschwei

@upodroid upodroid Awaiting requested review from upodroid

+1 more reviewer

@ReToCode ReToCode ReToCode approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@matzew matzew

@ReToCode ReToCode

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mgencur @ReToCode @matzew