-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🚨 Snowflake produces permanent tables 🚨 #9063
Conversation
4980acb
to
54da45c
Compare
/test connector=bases/base-normalization
|
/test connector=connectors/destination-snowflake
|
/test connector=bases/base-normalization
|
/test connector=connectors/destination-snowflake
|
b3a8ea3
to
24643b7
Compare
1292ebb
to
d8abb0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for providing the reading order, it was super helpful
@@ -0,0 +1,33 @@ | |||
FROM fishtownanalytics/dbt:0.21.1 | |||
COPY --from=airbyte/base-airbyte-protocol-python:0.1.1 /airbyte /airbyte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to get this docker image anymore? can't we just pip install airbyte-cdk
which contains the CDK models? not blocking but a good sweep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the COPY command equivalent to that pip install? (looks like the the docker image just contains base_python_structs
)
@@ -79,6 +79,8 @@ | |||
|
|||
public abstract class DestinationAcceptanceTest { | |||
|
|||
private static final String NORMALIZATION_VERSION = "dev"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
@@ -0,0 +1,33 @@ | |||
FROM fishtownanalytics/dbt:0.21.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we creating a special dockerfile for snowflake because it's DBT yamls deviate from the standard yamls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. Chris suggested modifying entrypoint.sh
to use the modified yaml instead of publishing a new docker image, but my thinking was to keep entrypoint.sh integration-agnostic.
Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
/publish connector=connectors/destination-snowflake |
/publish connector=connectors/destination-snowflake
|
/publish connector=bases/base-normalization
|
Closes #6743 |
What
Based on #9072
Update Snowflake's normalization implementation to create permanent (i.e. non-transient) tables.
Tests are failing with permissions errors - they were passing a few days ago locally, but I've never actually gotten them to pass on CI.
How
Actual changes:
normalization-snowflake
image, which sets the+transient: false
flag in dbt_project.ymldestination-snowflake
tonormalization-snowflake
in the workerMisc stuff:
normalization_test_output
diffs:Recommended reading order
🚨 User Impact 🚨
sort of - new tables will be created as permanent, so users who want to continue creating transient tables will need to create a new transient database for airbyte
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described here