feat: Dynamic tables and introduce proto versioning #610
Conversation
hermanschaaf
left a comment
hermanschaaf
left a comment
There was a problem hiding this comment.
Looks great! I had a look through the code. @yevgenypats Have you been able to use it successfully for a source plugin? It would be nice to see an example of it in use to better judge the interface and make sure it can be used to do what we need, before committing to it :)
Yes, definitely. What I'd like to do is the following:
|
erezrokah
left a comment
erezrokah
left a comment
There was a problem hiding this comment.
Looks good but it's quite a big diff to review.
To confirm, other than the new package structure the changes are:
- Add the new option
- Sync the dynamic tables during init
- Continue with sync based on dynamic tables
Is that mostly correct?
| return status.Errorf(codes.InvalidArgument, "failed to decode spec: %v", err) | ||
| } | ||
| ctx := stream.Context() | ||
| if err := s.Plugin.Init(ctx, spec); err != nil { |
There was a problem hiding this comment.
Was this missing or moved from someplace else?
There was a problem hiding this comment.
Yeah now we need to call it in v0 as we don't want to break the v0 protocol
|
|
||
| // Sync is syncing data from the requested tables in spec to the given channel | ||
| func (p *Plugin) Sync(ctx context.Context, spec specs.Source, res chan<- *schema.Resource) error { | ||
| func (p *Plugin) Init(ctx context.Context, spec specs.Source) error { |
There was a problem hiding this comment.
Ah, got it we need a separate Init to sync dynamic tables
I can split to PRs if that's helpful with one only introducing v0 moving things around and then one introducing v1. |
No need to split, just wanted to confirm |
hermanschaaf
left a comment
hermanschaaf
left a comment
There was a problem hiding this comment.
LGTM, not sure why the build is failing on the post-step
⏱️ Benchmark resultsComparing with 3756780
|
Import paths and references will need to be updated after #610 anyway, let's use this opportunity to change from e.g. `destination.WithDestinationLogger(log.Logger)` to `destination.WithLogger(log.Logger)` ~I will update the tests momentarily...~ done
This adds discovery service to support multiple versions. Follow-up to this: #610
🤖 I have created a release *beep* *boop* --- ## [1.28.0](v1.27.0...v1.28.0) (2023-01-23) ### Features * Add version discovery service ([#619](#619)) ([33ab32a](33ab32a)) * Dynamic tables and introduce proto versioning ([#610](#610)) ([448232c](448232c)) ### Bug Fixes * **clients:** Update `log line too long` message ([#611](#611)) ([0d3ff48](0d3ff48)) * Simplify client naming conventions ([#617](#617)) ([38b136b](38b136b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This is more of hot fix/patch to #610. #610 had an unwanted side effect on the migrate command, as now we init the plugin before running it. initializing the plugin is expensive, requires credentials and does a whole lot of validations, for example: https://github.com/cloudquery/cloudquery/blob/a06139efad9a2e8de14f186a223222c9d52ce58a/plugins/source/aws/client/client.go#L383 https://github.com/cloudquery/cloudquery/blob/a06139efad9a2e8de14f186a223222c9d52ce58a/plugins/source/azure/client/client.go#L124 Those seem unnecessary for tables migration (unless one uses dynamic tables). ---
This introduce WithDynamicTables for source plugins to give an option for plugins to generate tables on the go, this can be useful in GA analytics, Salesforce and others where you can only get the schema and tables in runtime.
Give WithDynamicTables is actually a breaking change and I didn't want to turn our code into spagetti that tries to understand in runtime which version is the client I took the opportunity to fix our protobuf structures and introduce a way for creating new versions for server in a breaking way while also serving older versions of the server so it wont affect user experience but will give us a better way to write and maintain different versions and eventually deprecate older versions.
cc @erezrokah as I know you suggested exactly that a while back but it was too early for me after the initial refactor :) and wasn't sure what are the requirements yet but here we go!