-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix CLI usage with no pre-set context/config #420
Conversation
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.
left some comments, but non-blocking
internal/storage/config.go
Outdated
_, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
return false, nil | ||
} | ||
return false, err | ||
} | ||
return true, nil |
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.
purely a style nit, but I'm used to seeing this written more like:
_, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)) | |
if err != nil { | |
if os.IsNotExist(err) { | |
return false, nil | |
} | |
return false, err | |
} | |
return true, nil | |
if _, err := os.Stat(filepath.Join(s.ConfigPath, configFileName)); errors.Is(err, fs.ErrNotExist) { | |
} else if err != nil { | |
} else { | |
} |
also apparently IsNotExist
is soft-deprecated in favor of errors.Is
: https://github.com/golang/go/blob/master/src/os/error.go#L88-L89
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.
I dig
return false, err | ||
} | ||
|
||
for _, token := range secrets.Tokens { |
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.
this is fine but makes me think Tokens
should just be a map[string]Token
instead of []Token
Also maybe this would make sense as a method on SecretStore
?
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.
Yeah, as I was going through this there were a bunch of things that seemed like they could be reorganized. I want to get the fix out and then loop back.
return storage.Token{}, err | ||
} | ||
if !configExists { | ||
return GetTokenWithCLIOverride(cmd, storage.Token{}) |
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.
shouldn't CLI override... override the context?
Co-authored-by: Evan Cordell <[email protected]>
Follow-on work around code organization can happen in the context of #421. |
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.
LGTM
Description
In #417 we introduced a bug where
zed
became unusable if there wasn't previously a context set by the user. This wasn't intentional; it should be valid to invoke zed entirely from the command line in isolation. This PR captures that behavior in a unit test and fixes the behavior.Changes
Testing
See that tests go green. See that you can clear out your local context (or run from within a container):
/zed schema write ./blah.zed --endpoint 127.0.0.1:50051 --token supersecretdevkey --insecure
and not receive an error.