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

Delete '/' at the end of url #1037

Closed
wants to merge 3 commits into from
Closed

Delete '/' at the end of url #1037

wants to merge 3 commits into from
+11 −2

Conversation

birariro
Copy link
Contributor

#707

Since the PR is not going on anymore
I added a test.

@OlgaMaciaszek
Copy link
Collaborator

Hello @birariro. Please see the related issue: #1070. This needs to be configurable. Let me know if you'd like to work on it. If not, I'll pick it up.

@OlgaMaciaszek
Copy link
Collaborator

Fixes gh-1070.

OlgaMaciaszek
OlgaMaciaszek requested changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @birariro. Please see the related issue: #1070. This needs to be configurable. Also, since you've picked changes over from gh-707, could you please cherry-pick them, so that the original commit is included.

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed waiting-for-triage labels Sep 27, 2024
birariro and others added 2 commits September 29, 2024 00:36
@birariro
code rollback
e067fdf
@artem-gorshkov @birariro
Delete '/' at the end of url
3b1d272 
In situation when we create FeignClient in this way:
@FeignClient(url = "https://localhost/", path = "api/v2")
We will get an error because of adding '/' in the beginning of the path in FeignClientsRegistrar.getPath, result will:
https://localhost//api/v2
To escape this situation we need to delete '/' at the end of url in FeignClientsRegistrar.getUrl
@birariro
Copy link
Contributor Author

@OlgaMaciaszek Done cherry-pick #707

@birariro
Copy link
Contributor Author

@OlgaMaciaszek

I checked the issue of #1070
For spring-projects/spring-framework, I think they want to use the correct URL
If so, shouldn't I use the correct URL when using SC OpenFeign?

@OlgaMaciaszek
Copy link
Collaborator

Hello @birariro, as noted here, this change needs to be configurable, by default disabled, in order to avoid breaking compatibility.

@birariro
Copy link
Contributor Author

Hello @OlgaMaciaszek

I've looked at the issue you've given me, but I think I'll have difficulty solving it with my ability.
Can you pick up the work?

@OlgaMaciaszek
Copy link
Collaborator

Sure, will do.

@OlgaMaciaszek
Copy link
Collaborator

Closing in favour of #1100.

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

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek requested changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@birariro @OlgaMaciaszek @spring-cloud-issues @artem-gorshkov