-
Notifications
You must be signed in to change notification settings - Fork 6
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
Set configuration options via environment variables #495
Comments
@DominicOram what is your preference? what if we have |
@callumforrester what was the reasoning to require it always in the first place? |
No preference, as long as it's well documented I'm not too fussed |
@stan-dot the configuration file is not always required per-se, but only when you want configuration that's different to the defaults. The intention was always to provide other ways of overriding configuration than a file, which is a bit clunky for a single value. That is what this ticket is for. For that reason, I have a preference for the second of @DominicOram's options. It's relatively common to only want to override one value, for example Eventually I would like to see blueapi use the same configuration override order that @tpoliaw has introduced to GDA:
|
So the long and short of it. Acceptance Criteria
|
This issue also relates to/could be done at the same time as #532 |
What is the rationale for this? it's more functionality exposed, and users might come to use all 4 in various combinations hard to debug. the Zen of Python says:
passing a file path is bulky, a long override in the cli also feels bulky
it's literally how most unix tools keep config, in the home directory in a hidden file, or in you say that a 'whole file is overkill', implying a spectrum of 'sizes' from a cli argument to file, where an etcd cache could be postulated as an extrapolation. I don't think the size is the relevant criterion here, but what is easy to work with. Files are easier to debug imo than env variables. One potential difference might be if the config has any secret fields, then in deployment some extra setup would be needed. |
Indeed, but the following feels like the least bulky solution: export BLUEAPI.config.stomp.host=localhost
blueapi serve You could easily just have this in a quickstart tutorial in the docs and it would take someone 5 seconds, as opposed to:
stomp:
host: localhost
|
this could all be addressed, but I concede for time savings now |
@callumforrester should the issue just be to do the whole override system with 4 layers? |
No, just environment variables (see acceptance criteria above) |
this issue is purely UX/DX, we could make it full from the first time. |
@DominicOram how exactly are you calling the CLI? I realized I'm not familiar how does |
This is just for manual setup and testing. We don't use it regularly in production so it's not a high priority for us |
and do you run it locally on beamline workstations or DH workstations or in the cluster? |
following @DiamondJoseph 's suggestion I will look into the pydantic setup taking this https://stackoverflow.com/questions/75709741/pydantic-nested-setting-objects-load-env-variables-from-file/75709846#75709846 into account |
Beamline workstations and DH workstations |
@callumforrester if I am to implement this, I must know some things:
|
@stan-dot I did not know the I have no strong opinions with the user-facing side as long as @DominicOram is happy. I am concerned that turning this into a big refactor of the existing config significantly increases the scope of this issue. Perhaps we need to break it down. For example:
Not quite sure if that's a sensible order to do things from reading the docs, thoughts welcome. |
First refactoring to use BaseSettings, then documenting how to pass overrides seems a sensible direction to me. Looks like Pydantic's default order is different from our expectations, but I think we take their order. |
thanks for the response @callumforrester
Yes, making the 'migrate to basesettings' as a separate issue is the best, and addressing this only after that is complete. |
unassigned as the basesettings PR was closed due to use case mismatch |
I would suggest we go with @DominicOram's other suggestion
Since the original plan has entailed a lot of unexpected complexity |
As a user trying to use the client with stomp it is frustrating that I have to provide the config file every time I call the CLI. It would be better if I could specify this as an environment variable so that I do not need to keep specifying it. We could do either:
export BLUEAPI.config.file=config_file
- this is the same sort of file we would currently useexport BLUEAPI.config.api.host=my_host
- we can then split out all the config fieldsThe text was updated successfully, but these errors were encountered: