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

vroom with ORS instance on different base path than /ors not possible #1036

Closed
TheGreatRefrigerator opened this issue Nov 14, 2023 · 5 comments

Comments

@TheGreatRefrigerator
Copy link
Contributor

TheGreatRefrigerator commented Nov 14, 2023

Hi 👋🏻 ORS maintainer here,

currently the base path in the ors_wrapper.cpp is hardcoded to /ors/v2. While this is the default of a self-hosted ors instance,
the live API doesn't use the /ors base path and no connection to it or other instances without the default base path is possible.

The solution would be to remove the hardcoded /ors basepath in the ors_wrapper.cpp. (the v2 is still the default but will change to v3 as well at some point.)
Since the introduction of the host.path the base path can now be set dynamically in the conifg.yml of vroom-express or passed via cli passing <profile>:<host>/ors.
And with the introduced change this would need to be done for coupling with selfhosted ors instances with default settings.

@nilsnolde
Copy link
Contributor

Important to note that this’ll break existing vroom/ORS users and they’ll need to append /ors now themselves. I highly doubt Julien would consider it a breaking change, but IMO this should be at least flagged in the changelog. Also vroom-express will need to get a new release with a new default config.yml to change the default host paths.

Good timing though with 1.14 being eminent apparently:)

@TheGreatRefrigerator
Copy link
Contributor Author

TheGreatRefrigerator commented Nov 14, 2023

but IMO this should be at least flagged in the changelog.

Agree! Will add to the changelog.

Also vroom-express will need to get a new release with a new default config.yml to change the default host paths.

I will add a PR that complements this.

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 14, 2023

I highly doubt Julien would consider it a breaking change

Hum right since it won't break anything for me. But users that suddenly get errors after upgrading might have a different opinion.

On the other hand if this is handled at the vroom-express level we should at least avoid complaints from the Docker users.

@TheGreatRefrigerator
Copy link
Contributor Author

VROOM-Project/vroom-express#99 the complementing PR is ready

@jcoupey jcoupey added this to the v1.14.0 milestone Nov 16, 2023
@jcoupey
Copy link
Collaborator

jcoupey commented Nov 16, 2023

Merged in master with #1037. I also cherry-picked the changes to include them in the release/1.14 branch. This will be part of the second release candidate I have to tag anyway (includes a regression fix). I'll also merge the vroom-express PR and tag a release over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants