Skip to content
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

More detailed API key error #921

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion pkg/config/profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"time"

log "github.com/sirupsen/logrus"
"github.com/spf13/viper"

"github.com/stripe/stripe-cli/pkg/validators"
Expand Down Expand Up @@ -135,7 +136,7 @@ func (p *Profile) GetAPIKey(livemode bool) (string, error) {
p.RegisterAlias(TestModeAPIKeyName, "api_key")
}

if err := viper.ReadInConfig(); err == nil {
if err = viper.ReadInConfig(); err == nil {
key = viper.GetString(p.GetConfigField(TestModeAPIKeyName))
}
} else {
Expand All @@ -153,6 +154,13 @@ func (p *Profile) GetAPIKey(livemode bool) (string, error) {
return key, nil
}

// If the log level is debug, return the raw error message. otherwise
// return a generic configuration message
level := log.GetLevel()
if level == log.DebugLevel {
return "", err
}

return "", validators.ErrAPIKeyNotConfigured
}

Expand Down
40 changes: 40 additions & 0 deletions pkg/config/profile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ import (
"io/ioutil"
"os"
"path/filepath"
"runtime"
"testing"

"github.com/sirupsen/logrus"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -101,6 +104,43 @@ test_mode_key_expires_at = '` + expiresAt + `'
cleanUp(c.ProfilesFile)
}

func TestAPIKeyLogLevel(t *testing.T) {
// Set the level to debug
logrus.SetLevel(logrus.DebugLevel)

c := &Config{
Color: "auto",
LogLevel: "debug",
Profile: Profile{
ProfileName: "tests",
TestModeAPIKey: "asdas",
},
ProfilesFile: "",
}

// For debug mode, the error should complain about a config file missing
// since we did not init the config
key, err := c.Profile.GetAPIKey(false)

// Little weird but windows returns a different error message than others
os := runtime.GOOS
switch os {
case "windows":
assert.ErrorContains(t, err, `config.toml: The system cannot find the file specified.`)
default:
assert.ErrorContains(t, err, `config.toml: no such file or directory`)
Copy link

@mnorth-stripe mnorth-stripe Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes these error messages change or vary (e.g., different linux distros) and I'd like us to have a very obvious and actionable signal if/when that happens, since we're effectively "scraping" a non-guaranteed API.

Can we have an assertion somewhere that alerts us if neither of the two cases we know about today are met?

}

assert.Equal(t, "", key)

// In info mode, it should give a cleaner error about the key not being
// configured
logrus.SetLevel(logrus.InfoLevel)
key, err = c.Profile.GetAPIKey(false)
assert.EqualError(t, err, "you have not configured API keys yet")
assert.Equal(t, "", key)
}

func helperLoadBytes(t *testing.T, name string) []byte {
bytes, err := ioutil.ReadFile(name)
if err != nil {
Expand Down