read project name from env by default

[xavdid-stripe]

Reviewers

r? @
cc @stripe/developer-products

Summary

Fixes #763. Uses viper's BindEnv to read STRIPE_PROJECT_NAME from the environment, if it exists. We then manually re-bind the env value to the Config, if needed.

Not sure how to write a unit test for this, but manual testing looks good. Testing using a tweaked version of config --list:

% stripe config --list
reading config default

% STRIPE_PROJECT_NAME=fromenv stripe config --list
reading config fromenv

% stripe config --list --project-name fromflag
reading config fromflag

% STRIPE_PROJECT_NAME=fromenv stripe config --list --project-name fromflag
reading config fromflag

NOTE This is the first production go code I've written, so extra scrutiny and tips is appreciated!

[xavdid-stripe]

Weirdly, the failing test passes on my machine 🤔

% go test -timeout 30s -run ^TestWriteProfile$ ./pkg/config
ok  	github.com/stripe/stripe-cli/pkg/config	0.207s

[xavdid-stripe]

Ah, the tests behave differently when run in isolation vs as part of make test 😵‍💫

$ make test
... fail

$ make test SOURCE_FILES=./pkg/config
ok!

When the tests in cmd are run, viper.Get("project-name") starts returning cobra's "default" value (instead of nil) I suspect that's why writeProfile takes an instance of viper.New().

I see there's a viper.Reset(), but calling it at the top of the config test doesn't seem to un-set whatever gets set in the root test.

I'm a little out of my depth, but I think we'll need either:

1. build a better division between test-viper and the one used in root.go
2. pass a viper pointer down to GetProfileName, which will default to the main one but can be overridden for tests. Seems bad to have that call be GetProfileName(nil) everywhere, but I think that'll work

What's the best way to proceed here?

[xavdid-stripe]

Had a chat w/ alan, figured out a solution here. Will update and flag for review.

[xavdid-stripe]

r?

[tomer-stripe]

Can you add tests for this? Changing the init has potential for a lode of unintended side effects in the future so I wan to make sure this doesn't break

Check out some of the afero fs ones we have floating around, it'll let you use the in memory fs which may be helpful.

[xavdid-stripe]

@tomer-stripe no problem! We added a couple of basic ones. The change itself is fairly small in scope (I think- feel free to correct), since we're only modifying specific keys. So, we added tests tracking changes to the Config object.

Let me know if there's anything else you'd like me to cover!

[vcheung-stripe]

Overall looks good to me other than when the project name doesn't exist, the error message is a little inaccurate. Any chance we could fix this?

STRIPE_PROJECT_NAME=asdf stripe customers list
You provided the "--project-name" flag, but no config for that project was found. Please run `stripe login --project-name=`..

[xavdid-stripe]

Sure can! Can you explain a little more what you mean by inaccurate? That's the same message that exists today when you provide a non-existant project in the flag:

stripe customers list --project-name asdf
You provided the "--project-name" flag, but no config for that project was found. Please run `stripe login --project-name=`...

What you you rather it say in that situation?

[vcheung-stripe]

It's inaccurate in that when I run STRIPE_PROJECT_NAME=asdf stripe customers list, the error message tells me that I provided the --project-name flag, but what I actually did was set an environment variable. I'm concerned this may cause confusion. Maybe we could say the analogous thing in this situation:

STRIPE_PROJECT_NAME=asdf stripe customers list
You set the STRIPE_PROJECT_NAME environment variable, but no config for that project was found. Please run `STRIPE_PROJECT_NAME=... stripe login`

[xavdid-stripe]

Oh got it! So maybe a message that is more ambiguous about where the project came from? Like You provided a project name (either via the "-project-name" flag or the "STRIPE_PROJECT_NAME" environment variable), but no config for that project was found ...

We could also try and suss out how they supplied the project name, but since they both point to the same variable, I'm not sure we could accurately determine how the value was provided.

[vcheung-stripe]

That's fair, let's go with your message!

[xavdid-stripe]

Sweet. Pushed e3e1063, which improves the message and took it a step further, adding the failed project name in the error message itself:

STRIPE_PROJECT_NAME=asdf stripe customers list
You provided the project name "asdf" (either via the "--project-name" flag or the "STRIPE_PROJECT_NAME" environment variable), but no config for that project was found.
Please run `stripe login --project-name=asdf` to enable commands for this project.
exit status 1

Let me know what you think!

[vcheung-stripe]

LGTM, thanks for making the changes!