Skip to content

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

Merged
merged 8 commits into from
Dec 13, 2021
Merged

Read config also from env #76

merged 8 commits into from
Dec 13, 2021

Conversation

polldo
Copy link
Contributor

@polldo polldo commented Dec 3, 2021

Motivation

to make it easier to execute arduino-cloud-cli commands from scripts, the cli should be able to fetch configuration credentials from environment variables
requested by @manchoz
overrides #65

Change description

the config will be 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

Additional Notes

Reviewer checklist

  • PR address a single concern.
  • PR title and description are properly filled.
  • Changes will be merged in main.
  • Changes are covered by tests.
  • Logging is meaningful in case of troubleshooting.
  • History is clean, commit messages are meaningful (see CONTRIBUTING.md) and are well formatted.

Copy link
Contributor

@glumia glumia left a 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)

@polldo
Copy link
Contributor Author

polldo commented Dec 7, 2021

in this way we will allow providing the configuration either through the environment variables or through the config file, but not both.

Yes, in the end I opted for this solution.
The trade-off I see here is: flexibility - usage complexity

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.
On the other hand, we would make throubleshooting more complex: imagine an user calling a command like config find to understand what config params he's using. Such command should have to return a detailed report specifying which parameters have been read in which config file (or env var).

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
If this is not enough @manchoz @eclipse1985 please let me know

(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)

this is already supported, environment variables are seen like a config file

Copy link
Contributor

@glumia glumia left a 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)

@polldo polldo merged commit 696a58f into main Dec 13, 2021
@polldo polldo deleted the polldo/config-env branch December 13, 2021 14:34
polldo pushed a commit that referenced this pull request Sep 2, 2022
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
polldo pushed a commit that referenced this pull request Sep 2, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants