refactor(cli): Update to managedplugin and decouple CLI config from proto #11655
Conversation
| return fmt.Errorf("failed to get source versions: %w", err) | ||
| } | ||
| maxVersion := findMaxVersion(versions) | ||
| if maxVersion >= 3 { |
There was a problem hiding this comment.
Shouldn't this be if maxVersion <= 3? Or am I missing something 🤔
There was a problem hiding this comment.
Oh no sorry, I get it now; it makes sense. This is when the source is using a newer protocol than the CLI supports.
There was a problem hiding this comment.
But in that case, if the source supported older versions, why don't we use them rather than only using the max?
There was a problem hiding this comment.
I'm also not sure how this works as the switch below only handles versions 2 and 1 (errors on 0 and default)
There was a problem hiding this comment.
@erezrokah Yeah I think this PR is not intended to handle SDK v3 yet; is that right @yevgenypats ? We may just want to update the title and description of this PR to make it clear this is mostly a refactor. Or are there any user-facing changes?
There was a problem hiding this comment.
Yeah I updated the title to refactor and removed v4. The only user facing change is that this CLI doesn't support an old version for sources which I dropped (source_v0). Once we move to single plugin protocol we can also have a schedule of how long we support a given protocol so we have a phase where we introduce a new protocol, add deprecation notice and then remove an old protocol support from the CLI. In any case users still can use an old version of the CLI.
Co-authored-by: Herman Schaaf <[email protected]>
yevgenypats
changed the title
| return fmt.Errorf("destination %s is used by multiple sources %s with different versions", destination, source.Path) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this validation step a new addition? I don't quite understand what it's doing / why we can't allow different source versions to use the same destination
There was a problem hiding this comment.
Looks like it's not new in any case; but I'm still curious why it exists 😄
we were exiting instead of continuing on migrating for multiple sources. Introduced here #11655
blocked by: https://github.com/cloudquery/plugin-pb-go/pull/16/files
Adds the following changes as pre-requirements that we can roll now and before migrating plugins to SDK V3:
managedplugincode (frommanagedsourcepluginandmanageddestinationplugin- now located atplugin-pb-go)BEGIN_COMMIT_OVERRIDE
fix(deps): Update
github.com/cloudquery/plugin-pb-gotov1.1.0END_COMMIT_OVERRIDE