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
refactor(config): RPC > HTTP & API for config #1671
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.
one nit then LGTM
lib/config.go
Outdated
@@ -55,37 +57,40 @@ func (m *ConfigMethods) GetConfig(p *GetConfigParams, res *[]byte) (err error) { | |||
if p.Field != "" { | |||
encode, err = cfg.Get(p.Field) | |||
if err != nil { | |||
return fmt.Errorf("error getting %s from config: %s", p.Field, err) | |||
return nil, fmt.Errorf("error getting %s from config: %s", p.Field, err) |
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.
gotta wrap them errors:
return nil, fmt.Errorf("error getting %s from config: %s", p.Field, err) | |
return nil, fmt.Errorf("error getting %s from config: %w", p.Field, err) |
lib/config.go
Outdated
return checkRPCError(m.inst.rpc.Call("ConfigMethods.GetConfig", p, res)) | ||
func (m *ConfigMethods) GetConfig(ctx context.Context, p *GetConfigParams) ([]byte, error) { | ||
if m.inst.http != nil { | ||
return nil, fmt.Errorf("config is not supported across nodes") |
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.
on second thought, this error is the one users will when they try to qri config get
while running qri connect
somewhere else. We should make this a standard error ErrUnsupportedRPC
so we can capture it with errors.Is
& present a clearer error for CLI users ("this could mean you're running qri connect in another terminal")
452e316
to
4ae9b66
Compare
lib/lib_test.go
Outdated
@@ -187,7 +187,7 @@ func TestReceivers(t *testing.T) { | |||
inst := &Instance{node: node, cfg: cfg} | |||
|
|||
reqs := Receivers(inst) | |||
expect := 10 | |||
expect := 9 |
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.
Before merging, need to rebase and drop this to 8.
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. We don't need config for Desktop, is that right?
AFAIK No just checked the codebase real quick, also Desktop communicates API only. |
Part of #1675