Serve UI from root #720
Serve UI from root #720
Conversation
|
|
||
| @app.on_event("startup") | ||
| async def create_webapp_dir_if_not_exists() -> None: |
There was a problem hiding this comment.
This function may still be useful to have. It's mostly for the case of someone is starting up the server for the first time and they've never built the frontend before, so don't have build/static/index.html on their local filesystem. on the fidesctl side, I added some lines to the Dockerfile to create these files, but even that doesn't work all the time since we frequently mount our own local directory over the built image.
it's always annoying to test this, but these are the two things I double checked for while working on this on the fidesctl side:
- delete your local
build/staticdirectory, then navigate to localhost:8080 and make sure it doesn't error out (I think without these lines it would, but am not positive, maybe it's more graceful than that!) - build and run the docker image without volume mounting to make sure that is getting the most up to date static files properly hosted at localhost:8080
if those two are working 💯 then this is probably good to go!
There was a problem hiding this comment.
confirmed deleting build/static and navigating to localhost:8080 causes a 500 error
There was a problem hiding this comment.
Good find. I'll add some code that handles that situation now
| class APIRouter(FastAPIRouter): | ||
| """ | ||
| Taken from: https://github.com/tiangolo/fastapi/issues/2060#issuecomment-834868906 | ||
| """ | ||
|
|
||
| def api_route( | ||
| self, path: str, *, include_in_schema: bool = True, **kwargs: Any | ||
| ) -> Callable[[DecoratedCallable], DecoratedCallable]: | ||
| """ | ||
| Updated api_route function that automatically configures routes to have 2 versions. | ||
| One without a trailing slash and another with it. | ||
| """ | ||
| if path.endswith("/"): | ||
| path = path[:-1] | ||
|
|
||
| add_path = super().api_route( | ||
| path, include_in_schema=include_in_schema, **kwargs | ||
| ) | ||
|
|
||
| alternate_path = path + "/" | ||
| add_alternate_path = super().api_route( | ||
| alternate_path, include_in_schema=False, **kwargs | ||
| ) | ||
|
|
||
| def decorator(func: DecoratedCallable) -> DecoratedCallable: | ||
| add_alternate_path(func) | ||
| return add_path(func) | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
Interesting solution, to handle trailing slashes. Main thing, we talked about a test for this one, otherwise, it's looking good
There was a problem hiding this comment.
Would echo what Dawn has said there. It's nice for the server to behave the same regardless of a trailing slash. Django for instance solves this with regex in the URL def where you can say something like:
re_path(r'^api/v1/some-endpoint/?', an_endpoint)
where the last ? makes the preceding / optional as it will trigger a match on both api/v1/some-endpoint and api/v1/some-endpoint/.
Since we're effectively doing this automatically now, it would be good to also document that somewhere in the API docs.
|
@pattisdr Everything should be good to go now. I added in unit tests to check that the new router is working and an API 404 check so API calls to non existent routes properly get a 404 instead of the admin ui However, the recent #811 PR moved some routes into I think it makes sense to get this merged now and address moving the new router into |
There was a problem hiding this comment.
Good work @TheAndrewJackson, since we need a slight tweak to the changelog, do you mind documenting that the endpoints work with both a trailing slash and non trailing slash per Sean's request?
CHANGELOG.md
Outdated
| ### Changed | ||
| * Update admin ui to be served from the root route `/` [#720](https://github.com/ethyca/fidesops/pull/720) | ||
|
|
There was a problem hiding this comment.
Small merging issue, move this up to "Unreleased"
There was a problem hiding this comment.
The changelog and docs have been updated. I put the note about trailing slashes in the API reference section. If that's not the best place and I can move it to another section.
There was a problem hiding this comment.
thank you Andrew, that looks fine!
Purpose
Update webserver to serve the admin-ui from the root (
/) instead of/static/index.htmlChanges
/and/{catchall:path}routes that handle serving all of the UI filesBASE_ASSET_URNconstant from the frontend because it's no longer needed/staticmounted directoryChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #666