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

Checking Check All Routes #2493

Closed
wants to merge 2 commits into from
Closed

Conversation

SteveDMurphy
Copy link
Contributor

Closes #2491

Code Changes

  • Add logging where trying to align production build of routes

Steps to Confirm

  • list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

Description Of Changes

This appears to only happen when using a production build of the front end, potentially due to the files being compiled in a different path than expected. Originally in 2.5.1

@SteveDMurphy
Copy link
Contributor Author

Some example paths:

/usr/local/lib/python3.10/site-packages/fides/ui-build/static/admin
fides/src/fides/ui-build/static/admin
src/fides/src/fides/ui-build/static/admin
.venv/lib/python3.10/site-packages/fides/ui-build/static/admin

We may need to look at handling an infinite number of preceding path combinations and look at something like fides/ui-build/static/admin

@SteveDMurphy
Copy link
Contributor Author

Took a few minutes to give this a little bit of a fresh look with what was discussed by @chriscalhoun1974, @sanders41, @Roger-Ethyca, and @seanpreston on the call tonight - it can definitely be improved with some decent coffee and some sleep but I think it might be a good start towards what we discussed! I was able to validate everything working as planned using nox -s "fides_env(test)"

@sanders41
Copy link
Contributor

I was going to have another look also, but got unexpected results. I ran nox -s "fides_env(test)", went to a page with a route after the / running on 8080, refreshed and got no error. I thought it was weird because I didn't think we had it fixed so I tried on main, again no error.

I do still get errors when using 1.6.0 from docker with docker pull ethyca/fides:2.6.0 and pip install ethyca-fides, but can no longer reproduce it running from the repo. I verified there were no new commits on main from when we were able to reproduce it earlier, and also used incognito mode to rule out cache.

@sanders41
Copy link
Contributor

sanders41 commented Feb 3, 2023

I did some further testing by connecting to the 1.6.0 docker container pulled from docker hub and modifying files in the container with the changes here. I found the pages are loading with this change in place. I am getting a 500 error 2023-02-03 03:50:41.683 [WARNING]: Error sending analytics event: The fideslog API responded with an error: 500 Internal Server Error, but this seems unrelated and maybe the logging server is down?

I was initially concerned this would only work when using our docker image and not when building a custom docker image or pip installing fides. To test this idea I deleted the src/fides/ui-build directory, leaving the ui-build in the /usr/local package directory to mimic a pip install ethyca-fides. Doing this the pages still loaded successfully so it seems as though this is working. We discussed multiple ways to start fides, and I haven't tested them all yet so there is a chance there are more failure cases still to be found.

@SteveDMurphy
Copy link
Contributor Author

I'll try and look at the fideslog stuff in a bit. Great catch! Even on failure we shouldn't surface anything if we can help it so am almost hoping it's something to do with the change...

@sanders41
Copy link
Contributor

Just to clarify a bit. the 500 error shows in the server log, but pages on the front end still loaded fine without error.

@SteveDMurphy
Copy link
Contributor Author

Ah cool, thank you!

@chriscalhoun1974
Copy link
Contributor

Executed nox -s dev on this branch. Admin UI running on http://localhost:3000 works as expected when performing a page refresh via F5 on the top and side level navigation routes.

@chriscalhoun1974
Copy link
Contributor

Just tested with nox -s "fides_env(dev)" and nox -s "fides_env(test)" commands. Page refresh is working as expected using http://localhost:3000.

@SteveDMurphy
Copy link
Contributor Author

Closing in favor of #2500

@NevilleS NevilleS deleted the SteveDMurphy-2491-check-all-bug branch May 16, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

browser refresh results in item not find
3 participants