Skip to content

Conversation

@erezrokah
Copy link
Member

@erezrokah erezrokah commented Feb 20, 2023

Summary

Extracted from #8156 as it requires an additional fix.
If the table doesn't have any PKs and in overwrite mode(s) we can only write a single resource per table as the doc ID will always be the same.
This PR changes it so if there are no PKs, we use all columns for the doc ID.

Discovered by the MigrateOverwrite test

@erezrokah erezrokah requested review from a team, hermanschaaf and shimonp21 and removed request for a team and shimonp21 February 20, 2023 14:22
@erezrokah erezrokah force-pushed the feat/update_elasticsearch_sdk branch from d058e8e to c9b5dd0 Compare February 20, 2023 14:24
@erezrokah erezrokah added the automerge Automatically merge once required checks pass label Feb 21, 2023
@candiduslynx candiduslynx removed the automerge Automatically merge once required checks pass label Feb 21, 2023
Copy link
Contributor

@candiduslynx candiduslynx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving but removing automerge bc the original author (@hermanschaaf) should take a look, too.

Copy link
Member

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@candiduslynx candiduslynx added the automerge Automatically merge once required checks pass label Feb 21, 2023
@kodiakhq kodiakhq bot merged commit 0e852ed into cloudquery:main Feb 21, 2023
kodiakhq bot pushed a commit that referenced this pull request Feb 21, 2023

#### Summary

Updates MongoDB to the new SDK with the following issues discovered in the tests:
- In overwrite mode, for a table with no PKs we used an empty filter always overwriting. The fix is similar to #8259 to use all columns as the filter
- When reading timestamp the Go lib uses local timezone. We want to read it as UTC (assuming it's written as UTC).

<!--
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants