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

Adding '/admin/' to default URL #23

Merged
merged 3 commits into from
Apr 13, 2023
Merged

Adding '/admin/' to default URL #23

merged 3 commits into from
Apr 13, 2023

Conversation

swastis10
Copy link
Contributor

When we are deploy the app to staging, it is being served from the /admin URL therefore we need to add url_base_pathname. Solution link : https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687

Testing done:

  1. Cognito works fine
  2. http://localhost:8050/admin/ is loading
  3. Other URLs such as http://localhost:8050/admin/tokens are also loading fine

When we are deploy the app to staging, it is being served from the /admin URL therefore we need to add `url_base_pathname`.
Solution link : https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687

Testing done:
1. Cognito works fine
2. `http://localhost:8050/admin/` is loading
3. Other URLs such as `http://localhost:8050/admin/tokens` are also loading fine
@swastis10 swastis10 requested a review from shankari April 12, 2023 21:46
@shankari
Copy link
Contributor

@swastis10 that is not actually the solution suggested in the link

The solution (at https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687/2) is to specify it once while setting up the dash app app = dash.Dash(requests_pathname_prefix='/myapp/') instead of putting it into every single URL path.

Is there a reason why you did not use that approach?

Also, you have indicated that the /admin path prefix works.
But have you tested with other path prefixes?
In particular, does it still work with no path prefix environment variable (using the default prefix of /)?

Finally, can you please ensure that we don't introduce extraneous whitespace changes. Look at the diffs for this PR - it is almost impossible to see what the real changes are since almost every line has supposedly changed.

This will also mess up the blame and make it harder to work with the code later.

@swastis10
Copy link
Contributor Author

@swastis10 that is not actually the solution suggested in the link

The solution (at https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687/2) is to specify it once while setting up the dash app app = dash.Dash(requests_pathname_prefix='/myapp/') instead of putting it into every single URL path.

Is there a reason why you did not use that approach?

Also, you have indicated that the /admin path prefix works. But have you tested with other path prefixes? In particular, does it still work with no path prefix environment variable (using the default prefix of /)?

Finally, can you please ensure that we don't introduce extraneous whitespace changes. Look at the diffs for this PR - it is almost impossible to see what the real changes are since almost every line has supposedly changed.

This will also mess up the blame and make it harder to work with the code later.

@shankari, I have used

app = Dash(
    external_stylesheets=[dbc.themes.BOOTSTRAP, dbc.icons.FONT_AWESOME],
    suppress_callback_exceptions=True,
    use_pages=True,
    url_base_pathname=url_path_prefix
)

Which loads the app at http://0.0.0.0:8050/admin/. Now if I go to the sidebar and click on, say, "Map" - It will redirect me towards http://0.0.0.0:8050/map/ which is not correct. Therefore, I have changed every single URL path to accomodate - http://0.0.0.0:8050/admin/map.

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

@shankari
Copy link
Contributor

shankari commented Apr 12, 2023

@swastis10 is there a reason that you are using url_base_pathname instead of the recommended requests_pathname_prefix from the answer? It looks like requests_pathname_prefix might prepend the pathname to all requests by default.

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

And you have tested this? Or you assume it based on the code?

@swastis10
Copy link
Contributor Author

requests_pathname_prefix

@shankari url_base_pathname seemed like a more appropriate choice as requests_pathname_prefix did not work.
From dash documentation:

url_base_pathname
A local URL prefix to use app-wide. Default '/'. Both requests_pathname_prefix and routes_pathname_prefix default to url_base_pathname. env: DASH_URL_BASE_PATHNAME

Also,

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

I have tested this

@shankari
Copy link
Contributor

shankari commented Apr 12, 2023

@swastis10 thank you for the clarification. Just to set expectations, it would have been helpful to state that upfront while creating the PR. Again, think about an intern looking at this code 10 years later - you have linked to a resource that says to use requests_pathname_prefix but you are not using it, wouldn't they want to know why?

And this is still not fully clear.
wrt requests_pathname_prefix did not work? What do you mean "did not work"?
Did it have the same behavior as url_base_pathname?
Because arguably url_base_pathname also did not work until you added the prefix to all the routes.
Please look at my issues as an example of how to indicate what worked and did not work

Finally, while I will merge this now given the time sensitivity, I still don't think that this is the correct solution.
The correct solution should be to specify the URL base while creating the app.
There are other URL paths, which we admittedly don't use a lot of of - css/javascript/images etc.
It is not a viable solution to go and change every single URL to have a prefix.

For the long term, you should indicate exactly why requests_pathname_prefix (with or without the default value) did not work and figure out how to get it to work. Notably, try https://dash.plotly.com/reference#dash.get_relative_path

Please file an issue for the cleanup task.

@swastis10
Copy link
Contributor Author

Adding the link to url_base_pathname

@shankari
Copy link
Contributor

I am merging this for now given the time sensitivity, but I don't think that it is the long-term solution.
@swastis10 please file an issue for the cleanup and I will merge

@swastis10
Copy link
Contributor Author

swastis10 commented Apr 12, 2023

@shankari shankari merged commit 21d4953 into dev Apr 13, 2023
@shankari
Copy link
Contributor

@swastis10 merged

@shankari shankari mentioned this pull request Apr 14, 2023
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.

2 participants