-
-
Notifications
You must be signed in to change notification settings - Fork 680
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.
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
base path support for openapi #3780
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
After enabling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! 🔥 Because the interplay between unleashUrl
and baseUriPath
is a little confusing, I think we need to make double sure that we're doing what we expect before merging this.
When working on this previously (#2079), I added a couple tests and even changed some of the expectations.
I'm a little too far removed right now to remember exactly the interplay between unleashUrl
and baseUriPath
, but in the previous PR, I made this assumption:
Best I can tell, the
baseUriPath
should be appended to theunleashUrl
to create the full path to Unleash. This seems to be different than what the existing tests assume. I also assume that ifunleashUrl
ends with a string that is equal tobaseUriPath,
then they should still be concatenated.
What do you think?
After enabling |
@thomasheartman I modified some of the tests. New logic is like this:
How to do the interplay of base path and unleash url with the same base path included can be changed later based on the actual use case we find. I tried to make minimal changes to our current code. |
After enabling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! Yeah, it's a good point: we can definitely adjust this later. If it does break the swagger ui requests, then that's easy enough to fix and definitely not critical. Looks great to me! 😄
Now that we have fixed the issue with server URLs prefixing the API paths (#3780), we can remove the cleaning script.
About the changes
Using full URL with base path in the dropdown which is different from proxy (where we only show the base path). TBH I don't know which one is better but decided to stick to full URL here because I don't know the implications of using just base path. Swagger should support both.
Important files
Discussion points