Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: preferAppend defaults to false if not defined #4088

Closed
wants to merge 12 commits into from

Conversation

fracasula
Copy link
Collaborator

@fracasula fracasula commented Nov 6, 2023

Description

Follow up to this PR where we changed the default behaviour for Warehouse destinations so that users would have to opt-in if they want to merge (aka remove duplicates).

Given that the UI changes for enabling the merge are not live we're setting enableMerge by default to true if it's not set. This maintains backwards compatibility and allows us to decouple the data plane deployment from the control plane one.

Linear Ticket

< Linear Link >

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@fracasula
Copy link
Collaborator Author

@achettyiitr @lvrach I'm proposing this approach. I have not updated the tests just yet, I'm just showcasing the changes here. Once I get a green light from everybody (product and CS as well) then I can update the tests. Let me know if it doesn't make sense somehow.

@fracasula fracasula marked this pull request as ready for review November 7, 2023 16:42
@fracasula fracasula changed the title chore: enableMerge default true chore: preferAppend default to false if not defined Nov 7, 2023
@fracasula fracasula changed the title chore: preferAppend default to false if not defined chore: preferAppend defaults to false if not defined Nov 7, 2023
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4610c7e) 71.98% compared to head (14cadbe) 71.96%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4088      +/-   ##
==========================================
- Coverage   71.98%   71.96%   -0.03%     
==========================================
  Files         372      372              
  Lines       54831    54838       +7     
==========================================
- Hits        39472    39464       -8     
- Misses      13059    13069      +10     
- Partials     2300     2305       +5     
Files Coverage Δ
warehouse/integrations/bigquery/bigquery.go 63.94% <100.00%> (-0.65%) ⬇️
warehouse/integrations/deltalake/deltalake.go 78.43% <100.00%> (ø)
warehouse/integrations/postgres/load.go 89.28% <100.00%> (+2.00%) ⬆️
warehouse/integrations/redshift/redshift.go 74.47% <100.00%> (+0.93%) ⬆️
warehouse/integrations/snowflake/snowflake.go 66.51% <100.00%> (ø)
warehouse/internal/model/warehouse.go 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fracasula fracasula requested a review from lvrach November 8, 2023 06:30
@fracasula fracasula changed the base branch from master to release/1.16.x November 8, 2023 10:18
@fracasula fracasula changed the base branch from release/1.16.x to master November 8, 2023 10:19
@fracasula
Copy link
Collaborator Author

Closing in favour of #4098

@fracasula fracasula closed this Nov 8, 2023
@fracasula fracasula deleted the chore/pipe-497 branch November 8, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants