feat: Implement transformations support.#18669
Merged
Merged
Conversation
erezrokah
reviewed
Jul 30, 2024
Member
erezrokah
left a comment
There was a problem hiding this comment.
Looks great 🚀 Added a few minor comments. Waiting for the migrate related changes and I'll do another pass cloudquery/plugin-pb#24
Comment on lines
+321
to
+327
| versions, err := transformer.Versions(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get transformer versions: %w", err) | ||
| } | ||
| if !slices.Contains(versions, 3) { | ||
| return fmt.Errorf("transformer plugin %[1]s does not support CloudQuery protocol version 3, required by the %[2]s source plugin. Please upgrade to a newer version of the %[1]s transformer plugin", transformer.Name(), source.Name) | ||
| } |
Member
There was a problem hiding this comment.
I don't think this can ever happen for transformer plugins, they are always written with protocol version 3
Contributor
Author
There was a problem hiding this comment.
Removed check.
Comment on lines
+328
to
+329
| destWarnings := specReader.GetTransformerWarningsByName(source.Name) | ||
| for field, msg := range destWarnings { |
Member
There was a problem hiding this comment.
Suggested change
| destWarnings := specReader.GetTransformerWarningsByName(source.Name) | |
| for field, msg := range destWarnings { | |
| transformerWarnings := specReader.GetTransformerWarningsByName(source.Name) | |
| for field, msg := range transformerWarnings { |
Member
There was a problem hiding this comment.
Let's also add a similar validation as in
to check if someone references a transformer that doesn't exit
Comment on lines
+237
to
+241
| pbClient, err := transformerPbClient.Transform(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| transformClientsByDestination[name] = append(transformClientsByDestination[name], pbClient) |
Member
There was a problem hiding this comment.
Suggested change
| pbClient, err := transformerPbClient.Transform(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| transformClientsByDestination[name] = append(transformClientsByDestination[name], pbClient) | |
| transformClient, err := transformerPbClient.Transform(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| transformClientsByDestination[name] = append(transformClientsByDestination[name], transformClient) |
erezrokah
approved these changes
Aug 1, 2024
kodiakhq Bot
pushed a commit
that referenced
this pull request
Aug 5, 2024
🤖 I have created a release *beep* *boop* --- ## [6.3.0](cli-v6.2.0...cli-v6.3.0) (2024-08-05) ### Features * Implement transformations support. ([#18669](#18669)) ([2af93ed](2af93ed)) * Support table name changes on basic transformer. ([#18833](#18833)) ([67d3701](67d3701)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.12.6 ([#18828](#18828)) ([3a48c89](3a48c89)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.57.1 ([#18830](#18830)) ([605c202](605c202)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.58.0 ([#18839](#18839)) ([6b57bca](6b57bca)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.58.1 ([#18852](#18852)) ([4320340](4320340)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for transformations in cli.
Transformations is a new property of the destination spec, e.g.:
Multiple transformers are supported on a per-destination basis. Thus, each destination now constructs a transformer pipeline:
This transformer pipeline is not some accidental complexity. I'd love it if we could just for-loop and apply transformations sequentially, but we decided to make transformers asynchronous, so a transformation is not some function we can call: it's a goroutine that has
recv&sendsemantics. I've abstracted the pipeline as simply as I could.Note that, even if we'd just allow a single transformer per destination as an MVP, we'd still need a goroutine for it (and thus for the source
Recvas well, which was a blockingfor { ... }up until now). So this is as close to MVP as I can make it.Current limitations
grpcat the moment.Transformerkind toplugin-pb-goin managed plugin yet. There's aTODOin the code whereDestinationis used instead, but I think at the moment it doesn't affect anything.Testing
In order to test this, you'll need a transformer. I've created https://github.com/cloudquery/cloudquery-private/tree/mariano/test-transformations which adds a test "reverser" transformer which you can
go run . serve, and you should be good to test a sync with the yaml from above.My testing
Syncs should continue to work as before if no transformations are added. Number of resources should not change between syncs (in case channel closing logic would lead to losing tails)
Adding a reverser transformer reverses the strings
I've also tried adding two reverses in sequence, which results in a regular sync (which tests that the pipeline system works). Note that currently you can't add the same transformer twice (we use a
map, which elides all but one instance), so I hacked it to try this. We might want to allow it.