feat(cli): Validate plugins gRPC protocol version#3657
feat(cli): Validate plugins gRPC protocol version#3657erezrokah wants to merge 4 commits intocloudquery:mainfrom
Conversation
| if err != nil { | ||
| return fmt.Errorf("failed to get protocol version from destination %q: %w", destPluginName+"@"+destPluginVersion, err) | ||
| } | ||
| if destProtocolVersion != expectedProtocolVersion { |
There was a problem hiding this comment.
Initially I thought about trying to be more specific (e.g. telling the user to update only the destination or only the source), but that probably has a ton of edge cases. For example what if we have protocol version 3, then we need to start to check ranges 🙃
48296ee to
89fef94
Compare
yevgenypats
left a comment
There was a problem hiding this comment.
I think let's wait with that. it should be done on the server side where there are still few culprits before we can take care of that.
Mostly we need to start versioning SDK with v1, v2 and making protobuf versioned as well (the package name in prtobouf) - this way v1 will never be able to connect to an uncompatible version.
We can always remove this logic when we have server side validation. This is mostly needed for users updating to the latest CLI version with the type system changes (we've seen reports on Discord with the error mentioned in the bug) |
|
Also the tests are blocked by cloudquery/plugin-sdk#351 but we can comment them |
|
PR to fix the Windows bug cloudquery/plugin-sdk#352 |
|
Im just working on this atm in my sdk pr cloudquery/plugin-sdk#348 so this will create more work for me reviewing and then removing the logic. let's hold off |
This is a split of this PR #348 so it will be easier to review. Also, Im going to push the move to v2 maybe another week as we dont really have anything that requires protocol changes for now. even though it is the right way to do it and will make the cli much easier including the need to things like that - cloudquery/cloudquery#3657 as it will be all handles on protobuf and go versioning side. This current PR just moves cqtypes to schema and removes enumerating all types in different locations to make sure when we add a new type tests will catch if a type is not added to the right place and or not connected to `TypeValue`. Also this removes `TimeInterval` which 1) wasn't used 2) it is incorrect because it used `time.Duration` as `TimeInterval` but it's not a TimeInterval it's just a duration which is uint64 so there no need for a special type for that. In PostgreSQL TimeInterval is actually completely different type that contains timezones and it is not go `time.Duration`
818e5eb to
c91c301
Compare
5469ddd to
57ae8b3
Compare
b79226c to
376012c
Compare
376012c to
f2db71d
Compare
|
Let's close this? |
I think it's not solved yet, but will be in the gRPC breaking change? But we can close for now |
Summary
Fixes #3628
Examples:
