Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Overload ReassignProxyRequest to also accept a route update #1760

Merged
merged 6 commits into from Jun 15, 2022
Merged

Overload ReassignProxyRequest to also accept a route update #1760

merged 6 commits into from Jun 15, 2022

Conversation

latelylk
Copy link
Contributor

Adds an overload for ReassignProxyRequest that assigns both route and cluster when reassigning a proxy request.

See discussions in #1749 and #1752 for more info.

@ghost
Copy link

ghost commented Jun 10, 2022 •
edited by ghost

CLA assistant check
All CLA requirements met.

@MihaZupan MihaZupan added this to the YARP 2.0.0 milestone Jun 13, 2022
MihaZupan
MihaZupan reviewed Jun 13, 2022
View reviewed changes
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thanks @ethankrein!
Could you please also add a test for the new overload (similar to the existing one)?

src/ReverseProxy/Model/HttpContextFeaturesExtensions.cs Outdated Show resolved Hide resolved
@latelylk
Copy link
Contributor Author

Could you please also add a test for the new overload (similar to the existing one)?

Is it okay that I extended that test to include the overload as well?

MihaZupan
MihaZupan approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Is it okay that I extended that test to include the overload as well?

That's fine by me, thanks.

test/ReverseProxy.Tests/Model/HttpContextFeaturesExtensions.cs Outdated Show resolved Hide resolved
@samsp-msft
Copy link
Contributor

@MihaZupan - are you going to merge before you go on vacation?

@MihaZupan
Copy link
Member

I want Chris to take a look at it first if this is the exact API he had in mind, but this is good to merge otherwise.

@Tratcher Tratcher self-assigned this Jun 15, 2022
@Tratcher Tratcher merged commit 5210218 into microsoft:main Jun 15, 2022
@Tratcher
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher approved these changes

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees

@Tratcher Tratcher

Labels
None yet
Projects
None yet
Milestone
YARP 2.0.0-RC1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@latelylk @samsp-msft @MihaZupan @Tratcher