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.

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

Merged
merged 5 commits into from
May 16, 2023
Merged

base path support for openapi #3780

merged 5 commits into from
May 16, 2023

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented May 16, 2023

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.
Screenshot 2023-05-16 at 09 10 15

Important files

Discussion points

@vercel
Copy link

vercel bot commented May 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) May 16, 2023 9:24am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) May 16, 2023 9:24am

@github-actions
Copy link

After enabling strictNullChecks this PR would be increasing the number of null check errors from to .
Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.

@kwasniew kwasniew changed the title remove unused file base path support for openapi May 16, 2023
Copy link
Contributor

@thomasheartman thomasheartman left a 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 the unleashUrl to create the full path to Unleash. This seems to be different than what the existing tests assume. I also assume that if unleashUrl ends with a string that is equal to baseUriPath, then they should still be concatenated.

What do you think?

@github-actions
Copy link

After enabling strictNullChecks this PR would be increasing the number of null check errors from to .
Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.

@kwasniew
Copy link
Contributor Author

@thomasheartman I modified some of the tests. New logic is like this:

  • if no base path is provided then do not set the servers section (which is used for base path settings)
  • if base path provided then do add a servers section with a UI dropdown

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.

@github-actions
Copy link

After enabling strictNullChecks this PR would be increasing the number of null check errors from to .
Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.

Copy link
Contributor

@thomasheartman thomasheartman left a 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! 😄

@kwasniew kwasniew merged commit d37bb6a into main May 16, 2023
9 checks passed
@kwasniew kwasniew deleted the base-path-swagger branch May 16, 2023 10:01
thomasheartman added a commit that referenced this pull request Jun 12, 2023
Now that we have fixed the issue with server URLs prefixing the API
paths (#3780), we can remove the cleaning script.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants