-
Notifications
You must be signed in to change notification settings - Fork 3
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 configuration of hostname and port #9
base: isochrone
Are you sure you want to change the base?
add configuration of hostname and port #9
Conversation
…e IT environment which blocks localhost
@@ -90,7 +90,7 @@ def report_time(): | |||
"Calculating costs for %s - %s", time_period.name, ", ".join(modes) | |||
) | |||
cost_settings = cost.CostSettings( | |||
server_url="http://localhost:8080", | |||
server_url="http://" + config.hostname + ":" + str(config.port), |
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.
Have you considered any url validation?
What would occur if a config hostname of "http://my_hostname" was provided by mistake?
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.
I'll add some extra validation on this next time I'm touching the code - have been a bit busy the last few weeks to pick it up ! |
No worries, I know the feeling @oweno-tfwm! I have just created a separate PR that includes some Url validation with If no values are specified in the config then the default configuration is loaded ("loalhost:8080"). Otherwise, values in the config are used and validated with AnyHttpUrl |
smashing - have a load of other changes in another fork that I've dropped you an email about (to the email address on your profile) |
add configuration of hostname and port so that it works in corporate IT environment that block use of 'localhost'