-
Notifications
You must be signed in to change notification settings - Fork 335
Settings: make global config file optional #225
Settings: make global config file optional #225
Conversation
Right now we crash in get_global_config() if the global config file does not exist. Removing this hard requirement, positions envs and the global config file as 2 alternative credential sources as one would expect. Relying on a global config file in the user's $HOME makes it awkward for CI, environment variables are usually prefferred especially for passing credentials.
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.
Thanks for your PR; I agree with your suggestion.
However, if the config doesn't exists on the fs we should make sure it is provided in env, instead of ignoring this misconfiguration.
I believe that with your change we won't ask the user to run wrangler config
because it's now optional, which is wrong.
If neither the config file or the envs provide the required values, the so with this change, incomplete settings will show the correct error, hinting to run We could probably improve the error message suggesting to run either |
@xtuc To sum it up the code already ensures that the config is not incomplete, by This change basically ensures that we throw an error only on incomplete settings, whereas prior to this change you would get an error if the global config file doesn't exist, despite providing valid config via ENVs. |
Thanks for the clarification. I'm not a fan of the way we merge the config. I think that we have two cases:
Misconfiguration should throw and be clearly communicated. However, I will see what the other reviews think about that. |
@xtuc Misconfiguration throws with a relatively clear error message (see https://github.com/cloudflare/wrangler/blob/3d4fa27329247427f5e2a16addd495bb2d65ae3b/src/settings/global_user.rs#L41) What behavior do you expect ? It would be good to get this merged, because it prevents users from using it in a CI setup (where you don't have an interactive terminal and it would be ugly to artificially generate a config file in CI) |
Thanks for the contribution, @AaronO! |
Right now we crash in
get_global_config()
if the global config file does not exist.Removing this hard requirement, positions envs and the global config file as 2 alternative credential sources as one would expect.
Relying on a global config file in the user's $HOME makes it awkward for CI, environment variables are usually preferred especially for passing credentials.