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

add support for url prefixes #109

Merged
merged 4 commits into from
Aug 20, 2021
Merged

add support for url prefixes #109

merged 4 commits into from
Aug 20, 2021

Conversation

jaimergp
Copy link
Member

Closes #98


  • Uses url_prefix in all blueprints
  • Needs a new blueprint for the custom routes provided by the authentication class. Might be a better way to handle this, specially since these URLs cannot be found through url_for. We should standardize the routes and corresponding method names in this case so we can use url_for.
  • The / route (index) is currently targeted by the list environments task. We need to use this in several instances (mainly due to the auth routes) in the templates to indicate "absolute path + prefix if any". If this method changes names or is assigned a different route, we need to manually update it. Maybe there's a way to alias a route to a 2nd method name that is just an alias?

@@ -71,16 +80,16 @@ def start(self):
app.secret_key = self.secret_key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route also needs to account for the url_prefix.

Copy link
Member

@costrouc costrouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the CORS bit this PR looks great. I also like how using the url_for makes the templates significantly more robust for routes changes. Thanks! I think me merging the docker clipboard PR your wrote first caused a merge conflict.

@jaimergp
Copy link
Member Author

Nice! CORS fixed.

I am not very proud of the /-index bits in the template, but we can iron those out in the future.

@costrouc
Copy link
Member

Yeah I think in the future the way to fix this is to have well defined names for these auth routes. Also I'd like to at some point make the auth provider produce a blueprint similar to what you did in app.py. It'd be nice to not have that dynamic magic. Big fan of the blueprint you added there.

@costrouc costrouc merged commit 520c7f3 into main Aug 20, 2021
@costrouc costrouc deleted the base-url branch August 20, 2021 18:20
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.

Support for base prefix to all routes
2 participants