-
-
Notifications
You must be signed in to change notification settings - Fork 4
Read config also from env #76
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
Conversation
9f96c96
to
a7f29de
Compare
9e40785
to
8fac0a6
Compare
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 skimmed through the changes and noticed one potential issue: in this way we will allow providing the configuration either through the environment variables or through the config file, but not both.
But what if I want to put my client ID in a config file and just the secret in the environment variables? (this is a common scenario, e.g. a project on Github)
If that's something we want to allow, we need to implement the configuration parsing from environment variables as something that overrides the config parsed from the file (eventually empty if no configuration files are found).
This would also allow us to extend the config file in the future (e.g. add a new option to configure CLI's default output format) without the need to add an environment variable for each new option.
(also, if we generalize a bit the implementation, we could allow users to specify any option of the config file through environment variables named ARDUINO_CLOUD_{option}
and solve the problem once and for all)
Yes, in the end I opted for this solution. If we allow an override mechanism (not only env over config file is possible, but also config file with higher priority over config file of less priority) we can support additional use cases like the one you described. In the end, I thought that supporting env vars only to allow the use of cli in scenarios in which the config file cannot be created (e.g. in CI) would have been enough
this is already supported, environment variables are seen like a config file |
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.
lgtm! (added just minor suggestions)
7f3ce0e
to
d277785
Compare
Allows the cli to fetch configuration credentials from environment variables. Config credentials can now be specified directly in `ARDUINO_CLOUD_CLIENT` and `ARDUINO_CLOUD_SECRET` environment variables. The config is now searched in this way: - if credentials are found on environment variables, return them - otherwise look for them in config files - if not found anywhere return an error
Allows the cli to fetch configuration credentials from environment variables. Config credentials can now be specified directly in `ARDUINO_CLOUD_CLIENT` and `ARDUINO_CLOUD_SECRET` environment variables. The config is now searched in this way: - if credentials are found on environment variables, return them - otherwise look for them in config files - if not found anywhere return an error
Motivation
to make it easier to execute
arduino-cloud-cli
commands from scripts, the cli should be able to fetch configuration credentials from environment variablesrequested by @manchoz
overrides #65
Change description
the config will be searched in this way:
Additional Notes
Reviewer checklist
main
.CONTRIBUTING.md
) and are well formatted.