Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create catch-all/fallback route for UI app #932
Create catch-all/fallback route for UI app #932
Changes from all commits
61590a0
ecff9ab
27ab92e
ebf8b66
6c28517
dd8fa8b
b40e428
046df62
b336d06
21dbbd9
45a61b0
5fc2fdc
c9f578e
ce8d3c2
3bae0cf
3745e2c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@peytondmurray I'm not entirely sure why, but this seems to be the line that is causing at least some of the CI checks to fail.
If you remove this line and go to localhost:8080/api/v1/namespace, you'll see that it redirects to same URL but with a forward slash at the end. If you then put this line back in and go to the same URL, you'll notice that it tries to load the client app. You get an HTML page back that tries to load the client app at that URL, but the client renders an error since it doesn't recognize that route (api/v1/namespace).
Any idea how to fix it?
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.
Recap
With line 22 removed:
With line 22 in place:
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.
This seems to suggest to me if FastAPI tries all the routes and cannot find a match, then it will add a trailing slash as a last-ditch attempt to not return a 404.
I see there is an option
redirect_slashes
for the FastAPI class whose default value is True.I will have to look at the FastAPI source code to see how and when it uses that option.
I wonder if the solution is to explicitly map both routes to the same handler. The following code would allow the user to get the internal admin UI via
/admin
or via/admin/
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.
I don't have context on this, but it seems like this is intended behavior on the FastAPI side of things. Others have wondered about how to prevent this and about the impacts of redirection on http[s].
Looking at the integration tests, the error shows that the URL is malformed:
It seems like this should be
http://localhost:8080/conda-store/api/v1/environment/?status=COMPLETED&artifact=CONDA_PACK&packages=python&packages=ipykernel
, no?