-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
Improve configurability #1654
base: master
Are you sure you want to change the base?
Improve configurability #1654
Conversation
Hi there @ianthetechie 👋, Thanks for these changes. Some thoughts on the different things included in this PR: config overridesThis has been a frequently discussed topic, and many people have asked for us to allow changing default settings in Despite our past stance on this I think your change is pretty simple and helpful, and so I'd support it being merged as long as we make it clear in the docs that any settings accessed this way are 100% "you break it, you get to buy the pieces". I don't love the idea of having to triage issue tickets only to find out people have ridiculous values for some default settings that they don't even fully understand in their Error handlingYeah, Pelias API error handling has always been bad. The most glaring issues were fixed shortly after we launched Geocode Earth back in 2018 (#1108, #1231), but there are still a bunch of cases where errors from services like Placeholder and Interpolation can cause Pelias to return a 4xx error when it should return 5xx. Can you split those changes into a new, clean PR and describe a bit more about exactly what you saw that caused the errors? We'd always want to track those down. shebangsI don't think explicit FreeBSD support is going to be a goal of Pelias any time soon, but it seems like a relatively reasonable change. We do in practice need to support Macs since most people will be developing on them. Same thing: new clean PR with just those changes please. |
Hi 👋 yep totally agree with what you said Julian.
If the settings can be improved then I would prefer to see everyone share their insights from testing & tweaking these settings in the spirit of open-source. It could be that each installation needs a different set of values or it could be that we work together to tune 'ideal' values.
|
👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.
Here's the reason for this change 🚀
This PR does not fix any open issues that we're aware of, but it adds the following features which we think are generally useful to upstream ;)
In particular, it makes the Pelias API 1) more configurable via tunables which can be customized per use-case, and 2) improves the correctness of HTTP status codes.
Here's what actually got changed 👏
/usr/bin
, that isn't the case on other UNIX variants (ex: FreeBSD, which I often develop on), and it's quite likely Apple won't ship bash at some point in the future, so using/usr/bin/env bash
will ensure portability.Here's how others can test the changes 👀
The PR contains unit tests demonstrating 502 responses on some real backend errors.
A straightforward approach to testing config overrides didn't immediately come to mind as I don't write JS very often, but here is an example fragment of a pelias.json config demonstrating the use if you want to test locally: