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

#2064 2065 #2132 #2169 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator #2225

Merged
merged 11 commits into from
Dec 10, 2024
Merged

#2064 2065 #2132 #2169 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator #2225

merged 11 commits into from
Dec 10, 2024

Conversation

int0x81
Copy link
Contributor

@int0x81 int0x81 commented Nov 28, 2024 •
edited by raman-m
Loading

@raman-m raman-m added conflicts Feature branch has merge conflicts Routing Ocelot feature: Routing and removed conflicts Feature branch has merge conflicts labels Dec 3, 2024
@raman-m
Copy link
Member

raman-m commented Dec 3, 2024

Thanks for the PR creation!
Why don't you request code reviews?

raman-m
raman-m requested changes Dec 3, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Our development process mandates the inclusion of acceptance tests for even the smallest fixes. Please refer to points 4 and 5.

So, please add acceptance tests!

@raman-m raman-m changed the title Fixed issue 2132 #2132 Ampersand workaround in UpstreamTemplatePatternCreator Dec 3, 2024
@raman-m raman-m changed the title #2132 Ampersand workaround in UpstreamTemplatePatternCreator #2132 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator Dec 3, 2024
@raman-m raman-m added the bug Identified as a potential bug label Dec 3, 2024
@int0x81 int0x81 mentioned this pull request Dec 4, 2024
Closed
raman-m
raman-m requested changes Dec 6, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment •
edited
Loading

Choose a reason for hiding this comment

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

$\color{red}{No\ acceptance\ tests!}$

Please include tests. If you're uncertain about where to locate the appropriate testing class within the Ocelot.AcceptanceTests project, please inform me.

Please add tests to Routing → RoutingTests

@raman-m
Copy link
Member

raman-m commented Dec 6, 2024

@ggnaegi Please join to review the Finn's coding art 😋

@int0x81
Copy link
Contributor Author

int0x81 commented Dec 6, 2024

N o a c c e p t a n c e t e s t s !

Please include tests. If you're uncertain about where to locate the appropriate testing class within the Ocelot.AcceptanceTests project, please inform me.

Please add tests to Routing → RoutingTests

@raman-m
I added an acceptance test based on the issue #2065

@int0x81 int0x81 requested a review from raman-m December 6, 2024 10:21
ggnaegi
ggnaegi requested changes Dec 6, 2024
View reviewed changes
Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

Hello, thanks a lot, it would be imho, more elegant to use a ternary operator, or even Regex.Replace

@int0x81 int0x81 requested a review from ggnaegi December 9, 2024 09:46
@raman-m
Copy link
Member

raman-m commented Dec 9, 2024

Reviewing and checking tests...

raman-m
raman-m requested changes Dec 9, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

A few issues should be resolved!

@raman-m
Copy link
Member

raman-m commented Dec 9, 2024 •
edited
Loading

@int0x81 Could you clarify this for me? 👇
image

Why have you restricted my ability to push to your forked repository?
I am not able to assist you with pair programming...

@raman-m
Copy link
Member

raman-m commented Dec 9, 2024 •
edited
Loading

@int0x81 Dear Finn,
Could you please respond to my question? I would like to push a small code review commit.

As per GitHub's policy on forking repositories, maintainers or owners of the base repository have write permission to the feature branches of the pull requests, but not to other branches, including protected ones.
Could you clarify what actions were taken with your forked repository that resulted in me not having push access?

@ggnaegi FYI

@int0x81
Copy link
Contributor Author

int0x81 commented Dec 10, 2024

@raman-m I usually disable the option Allow edit by maintainers, because I like to have control over my changes and avoid confusion. Please don't take it personally :D. However, I just enabled it so you can push changes, but please keep your changes small so I can follow up on them.

raman-m
raman-m approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Ready for Delivery ✅

@raman-m raman-m added the NET9 .NET 9 release label Dec 10, 2024
@raman-m raman-m added this to the .NET 9 milestone Dec 10, 2024
@raman-m raman-m changed the title #2132 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator #2064 2065 #2132 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator Dec 10, 2024
@raman-m raman-m changed the title #2064 2065 #2132 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator #2064 2065 #2132 #2169 Ampersand vs Slash workaround in UpstreamTemplatePatternCreator Dec 10, 2024
@raman-m raman-m linked an issue Dec 10, 2024 that may be closed by this pull request
Bug with catch-all routing #2169
Closed
@raman-m
Copy link
Member

raman-m commented Dec 10, 2024 •
edited
Loading

Hello Finn,
I've prioritized this PR for the upcoming .NET 9 release, so it's now ready for merging.
If you're okay with the minor enhancements in b7853e0, feel free to press the magic Squash and merge button 😉
Please avoid using the Create a merge commit option❗

@int0x81
Copy link
Contributor Author

int0x81 commented Dec 10, 2024

Hello Finn, I've prioritized this PR for the upcoming .NET 9 release, so it's now ready for merging. If you're okay with the minor enhancements in b7853e0, feel free to press the magic Squash and merge button 😉 Please avoid using the Create a merge commit option❗

@raman-m

I don't think I am authorized to merge. The button is disabled for me. But I reviewed the changes and it looks good to me, so you can also perform the merge directly

@raman-m raman-m merged commit 995b103 into ThreeMammals:develop Dec 10, 2024
1 check passed
@raman-m
Copy link
Member

raman-m commented Dec 10, 2024

I don't think I am authorized to merge. The button is disabled for me.

Yes, right! Only team members can merge to protected branches. Done! Merged!
Thank you for your contribution!

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

@raman-m raman-m raman-m approved these changes

@ggnaegi ggnaegi ggnaegi approved these changes

@RaynaldM RaynaldM Awaiting requested review from RaynaldM

Assignees

@int0x81 int0x81

Labels
bug Identified as a potential bug NET9 .NET 9 release Routing Ocelot feature: Routing
Projects
None yet
Milestone
.NET 9
3 participants
@int0x81 @raman-m @ggnaegi