-
Notifications
You must be signed in to change notification settings - Fork 89
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
Allow for customization of path_prefix_server by setting Turbine field. #292
base: main
Are you sure you want to change the base?
Allow for customization of path_prefix_server by setting Turbine field. #292
Conversation
from free function to Turbine public method. This way, path_prefix_server can be customized in order to build non-standard combinations of multiple Perseus apps.
Hey, sorry for the late reply, I've been really busy lately, but I am back now! I think this is a good idea in theory, but I don't love the idea of moving this into the code. From my perspective, the base path is very much a configuration value, and implanting that into an app's code isn't amenable to the philosophy of keeping configuration that can change easily (e.g. an app should be able to be hosted wherever you like once the code is ready). Also, to clarify, your use-case for this is running two turbines in the same server? Is there any particular reason you can't run the two as separate processes? |
I get your point, however several other "configurations" are currently available for customization in code. I believe this is for good reason: instead of wiring the developer onto a specific way of managing configuration (environment vars), the developer should be free to decide for, say, a configuration file. |
Okay, I certainly accept that, and your use-case is far less niche than I was expecting! Let me think about this some more and review your code in greater depth. |
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.
Very minor wording change in one comment, otherwise this is good to merge!
I've thought about it a bit more and I think, given this is such an important configuration element, it's probably a good idea to increase the flexibility of things by having it be an option that can be specified in the code. Thanks very much for the suggestion and the PR!
Co-authored-by: Sam Brew <[email protected]>
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.
Looks great, thank you so much!
@giacomocariello would you mind resolving the conflicts with |
Currently only a single Perseus app can be served by a single server backend. While Perseus already allows for customization of app base path, this customization is activated by setting PERSEUS_BASE_PATH environment variable, which means that the same path prefix is applied process-wide, so it's not possible to create a non-standard server implementation "mounting" two Turbines on different URL mountpoints.
This means that the only way to have multiple Perseus apps running under the same origin is to use a reverse proxy to forward requests to different Perseus servers.
This PR is meant to allow setting a path_prefix_server on a Turbine, while leaving the default behavior of resolving PERSEUS_BASE_PATH environment variable if a custom value is not set.