fix: Make CQTypes to Schema and introduce versioning.#348
Closed
yevgenypats wants to merge 16 commits intomainfrom
Closed
fix: Make CQTypes to Schema and introduce versioning.#348yevgenypats wants to merge 16 commits intomainfrom
yevgenypats wants to merge 16 commits intomainfrom
Conversation
Merged
hermanschaaf
approved these changes
Nov 7, 2022
Contributor
hermanschaaf
left a comment
There was a problem hiding this comment.
LGTM 👍 Only thing is that we should probably mark this as fix! so that the version gets bumped to the next major version?
| Name: "dur_pointer_col", | ||
| Type: schema.TypeTimeInterval, | ||
| Resolver: `schema.PathResolver("DurPointerCol")`, | ||
| }, |
Contributor
There was a problem hiding this comment.
Maybe I'm missing some context, but why are the duration column tests being removed?
| // need to check for duration here, as Go time.Duration is int64 | ||
| if v == reflect.TypeOf(time.Duration(0)) { | ||
| return schema.TypeTimeInterval, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Oh I see it's also removed here. Could this break an existing plugin by any chance?
| if err := json.Unmarshal(res[i]["type"], &t); err != nil { | ||
| return err | ||
| } | ||
| if t <= TypeInvalid || t >= TypeEnd { |
Contributor
There was a problem hiding this comment.
Interesting technique to check t >= TypeEnd, but I like it 😄
Member
There was a problem hiding this comment.
it's very C (MAX_TYPE would be the C counterpart)
yevgenypats
added a commit
that referenced
this pull request
Nov 7, 2022
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`
disq
reviewed
Nov 7, 2022
| rpc GetSyncSummary(GetSyncSummary.Request) returns (GetSyncSummary.Response); | ||
| // Fetch resources | ||
| rpc Sync(Sync.Request) returns (stream Sync.Response); | ||
| // Sync2 is a new sync API that supports CQ Types. It is not backward compatible with Sync. |
Member
There was a problem hiding this comment.
Doc looks a bit outdated, should explain by itself not referring to older sync
Contributor
Author
|
Closing in favour of #369 |
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 8, 2022
Closes #348 Introduce testing suite for destination plugins as well as number of methods to including `transformers` and `reverseTransformers` to help testing suite be able to test "end-to-end". This also adds more testing with a `memoryDB` plugin to test destination plugin and destination plugin testing suite
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
4 participants
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.
This make the following changes:
ValueTypereside inside the same package as the types. Given I didn't want to update all the plugins I moved the types intoschemaand not the otherway around/v1)TransformerandReverseTransformerfor destination plugins. This way it is easy for the SDK to have round trip tests with those transformers and theReadcapability.