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

Fix CLI usage with no pre-set context/config #420

Merged
merged 12 commits into from
Sep 19, 2024

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Sep 19, 2024

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

  • Separate existence and fetching concerns for tokens
  • Handle cases where config file or token don't exist

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.

@tstirrat15 tstirrat15 changed the title Fix CLI usage with no pre-set context Fix CLI usage with no pre-set context/config Sep 19, 2024
ecordell
ecordell previously approved these changes Sep 19, 2024
Copy link
Contributor

@ecordell ecordell left a 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/client/client_test.go Outdated Show resolved Hide resolved
Comment on lines 152 to 159
_, err := os.Stat(filepath.Join(s.ConfigPath, configFileName))
if err != nil {
if os.IsNotExist(err) {
return false, nil
}
return false, err
}
return true, nil
Copy link
Contributor

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:

Suggested change
_, 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

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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{})
Copy link
Member

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?

@tstirrat15
Copy link
Contributor Author

Follow-on work around code organization can happen in the context of #421.

Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@tstirrat15 tstirrat15 merged commit 46db2fe into main Sep 19, 2024
15 checks passed
@tstirrat15 tstirrat15 deleted the fix-usage-with-no-context branch September 19, 2024 22:53
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
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.

3 participants