Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Settings: make global config file optional #225

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Jun 6, 2019

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.

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.
xtuc
xtuc previously requested changes Jun 7, 2019
Copy link
Member

@xtuc xtuc left a 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.

@AaronO
Copy link
Contributor Author

AaronO commented Jun 7, 2019

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 .try_into() part will fail (because the api_key and email attributes of GlobalUser are not optional Option<...>): https://github.com/cloudflare/wrangler/blob/3d4fa27329247427f5e2a16addd495bb2d65ae3b/src/settings/global_user.rs#L36-L47

so with this change, incomplete settings will show the correct error, hinting to run wrangler config.

We could probably improve the error message suggesting to run either wrangler config or set $CF_EMAIL and $CF_API_KEY

@AaronO
Copy link
Contributor Author

AaronO commented Jun 7, 2019

@xtuc To sum it up the code already ensures that the config is not incomplete, by matching against the Result<> of .try_into().

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.

@xtuc
Copy link
Member

xtuc commented Jun 7, 2019

Thanks for the clarification. I'm not a fan of the way we merge the config. I think that we have two cases:

  • CI, where you can provide config in env
  • Desktop, where you can provide the config in a file

Misconfiguration should throw and be clearly communicated. However, I will see what the other reviews think about that.

@AaronO
Copy link
Contributor Author

AaronO commented Jul 2, 2019

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

@ashleygwilliams ashleygwilliams merged commit 06a29a4 into cloudflare:master Jul 29, 2019
@kristianfreeman
Copy link
Contributor

Thanks for the contribution, @AaronO!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants