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

feat: Upgrade to SDK v4 #7

Merged
merged 9 commits into from
Nov 6, 2023

Conversation

maaarcelino
Copy link
Contributor

Hi there! This is a PR to update to our latest SDK! We are also rolling out hub shortly - https://hub.cloudquery.io so, once this PR is approved we can follow-up with additional PR so publish the plugin to our hub so users can enjoy dynamic searchable documentation for all versions and all tables.

plugin/plugin.go Outdated
)

var (
Kind = "source"
Team = "coinpaprika"
Name = "coinpaprika-coinpaprika"

Choose a reason for hiding this comment

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

Suggested change
Name = "coinpaprika-coinpaprika"
Name = "coinpaprika"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f40bcbd

@@ -52,7 +52,7 @@ func fetchTickers(ctx context.Context, meta schema.ClientMeta, parent *schema.Re
startDate := cl.StartDate
key := fmt.Sprintf(stateKeyTpl, *c.ID)
if cl.Backend != nil {
value, err := cl.Backend.Get(ctx, key, cl.ID())
value, err := cl.Backend.GetKey(ctx, key)

Choose a reason for hiding this comment

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

IMO you could just use state.NopClient & never perform the if cl.Backend != nil check

Choose a reason for hiding this comment

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

still here

return fmt.Errorf("set state failure: %w", err)
return fmt.Errorf("failed to save state to backend: %w", err)
}
err = cl.Backend.Flush(ctx)

Choose a reason for hiding this comment

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

no need to perform manual flush, done in plugin.Client.Sync

plugin/client.go Outdated
type Client struct {
logger zerolog.Logger
config client.Spec
backendConn *grpc.ClientConn

Choose a reason for hiding this comment

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

Suggested change
backendConn *grpc.ClientConn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f40bcbd

plugin/client.go Outdated
Comment on lines 33 to 35
if c.backendConn != nil {
return c.backendConn.Close()
}

Choose a reason for hiding this comment

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

Suggested change
if c.backendConn != nil {
return c.backendConn.Close()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f40bcbd

}

func (c *Client) Sync(ctx context.Context, options plugin.SyncOptions, res chan<- message.SyncMessage) error {
tt, err := c.tables.FilterDfs(options.Tables, options.SkipTables, options.SkipDependentTables)

Choose a reason for hiding this comment

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

plugin/client.go Outdated
Comment on lines 53 to 74
var stateClient state.Client
if options.BackendOptions == nil {
c.logger.Info().Msg("No backend options provided, using no state backend")
stateClient = &state.NoOpClient{}
c.backendConn = nil
} else {
c.backendConn, err = grpc.DialContext(ctx, options.BackendOptions.Connection,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultCallOptions(
grpc.MaxCallRecvMsgSize(maxMsgSize),
grpc.MaxCallSendMsgSize(maxMsgSize),
),
)
if err != nil {
return fmt.Errorf("failed to dial grpc source plugin at %s: %w", options.BackendOptions.Connection, err)
}
stateClient, err = state.NewClient(ctx, c.backendConn, options.BackendOptions.TableName)
if err != nil {
return fmt.Errorf("failed to create state client: %w", err)
}
c.logger.Info().Str("table_name", options.BackendOptions.TableName).Msg("Connected to state backend")
}

Choose a reason for hiding this comment

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

Suggested change
var stateClient state.Client
if options.BackendOptions == nil {
c.logger.Info().Msg("No backend options provided, using no state backend")
stateClient = &state.NoOpClient{}
c.backendConn = nil
} else {
c.backendConn, err = grpc.DialContext(ctx, options.BackendOptions.Connection,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultCallOptions(
grpc.MaxCallRecvMsgSize(maxMsgSize),
grpc.MaxCallSendMsgSize(maxMsgSize),
),
)
if err != nil {
return fmt.Errorf("failed to dial grpc source plugin at %s: %w", options.BackendOptions.Connection, err)
}
stateClient, err = state.NewClient(ctx, c.backendConn, options.BackendOptions.TableName)
if err != nil {
return fmt.Errorf("failed to create state client: %w", err)
}
c.logger.Info().Str("table_name", options.BackendOptions.TableName).Msg("Connected to state backend")
}
stateClient := state.Client(&state.NoOpClient{})
if options.BackendOptions != nil {
conn, err := grpc.DialContext(ctx, options.BackendOptions.Connection,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultCallOptions(
grpc.MaxCallRecvMsgSize(maxMsgSize),
grpc.MaxCallSendMsgSize(maxMsgSize),
),
)
if err != nil {
return fmt.Errorf("failed to dial grpc destination plugin at %s: %w", options.BackendOptions.Connection, err)
}
stateClient, err = state.NewClient(ctx, conn, options.BackendOptions.TableName)
if err != nil {
return fmt.Errorf("failed to create state client: %w", err)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f40bcbd

@@ -52,7 +52,7 @@ func fetchTickers(ctx context.Context, meta schema.ClientMeta, parent *schema.Re
startDate := cl.StartDate
key := fmt.Sprintf(stateKeyTpl, *c.ID)
if cl.Backend != nil {
value, err := cl.Backend.Get(ctx, key, cl.ID())
value, err := cl.Backend.GetKey(ctx, key)

Choose a reason for hiding this comment

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

still here

@@ -93,9 +93,9 @@ func fetchTickers(ctx context.Context, meta schema.ClientMeta, parent *schema.Re
res <- tt
}
if cl.Backend != nil {
err = cl.Backend.Set(ctx, key, cl.ID(), upTo.Format(time.RFC3339))
err = cl.Backend.SetKey(ctx, key, upTo.Format(time.RFC3339))

Choose a reason for hiding this comment

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

remove if cl.Backend != nil check

@candiduslynx
Copy link

cc: @mbillewicz @r--w

@r--w
Copy link
Member

r--w commented Nov 2, 2023

Wow, thanks for this contribution; it seems like you spent some time on this PR. We will review the code within a week.

@r--w
Copy link
Member

r--w commented Nov 2, 2023

Hey @candiduslynx & @maaarcelino could you fix the quality/linter issues?

You can run this command locally make check

@maaarcelino
Copy link
Contributor Author

@r--w There's only a make lint command which passes for me locally without making any changes. Those logs appear to point to some other code (e.g. deleteValue is not present in any of the files in this repo).

There were a few failing tests due to a nil pointer and I've fixed those. Now both make lint and make test all pass.

@mbillewicz
Copy link
Collaborator

mbillewicz commented Nov 3, 2023

@maaarcelino Try to update linter in .github/workflows/test.yaml line 26 to latest version.

@maaarcelino
Copy link
Contributor Author

maaarcelino commented Nov 3, 2023

@mbillewicz Updated to 1.55.1 and added skip for build/pkg folders as the linting included folders outside the project

@mbillewicz
Copy link
Collaborator

@maaarcelino pls also regenerate files by make gen due to failing workflow test job

@r--w r--w merged commit d9af40e into coinpaprika:master Nov 6, 2023
1 check passed
@r--w
Copy link
Member

r--w commented Nov 6, 2023

@maaarcelino merged - thank you.

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

Successfully merging this pull request may close these issues.

4 participants