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

VTN-29032, create interface to graphql client #20

Merged
merged 1 commit into from Aug 29, 2019

Conversation

ctsiokos-veritone
Copy link

VTN-29032
This is to allow us to use mocks in the Core API layer of Edge services.

Run(ctx context.Context, req *Request, resp interface{}) error
SetLogger(func(string))
}

// Client is a client for interacting with a GraphQL API.

Choose a reason for hiding this comment

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

This comment should be describing the interface on line 51

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, I didn't catch your comment prior to the merge. I would prefer deleting something as obvious as this.

Copy link

@tnguyen-veri tnguyen-veri left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -48,25 +48,26 @@ import (
"github.com/pkg/errors"
)

type Client interface {

Choose a reason for hiding this comment

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

This change caused a problem in my repo where I was using a * to graphql.Client...!!

Choose a reason for hiding this comment

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

Same issue with edge-output-writer https://github.com/veritone/edge-output-writer/blob/master/api/graphql.go#L78 where I was using

Copy link
Author

Choose a reason for hiding this comment

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

The clients of graphql need to be updated to use the new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants