-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Un-revert add /tmp emptyDir volume to connector pods #11511
Conversation
42b73ec
to
e07962c
Compare
da67ab7
to
3d979e1
Compare
3d979e1
to
78e608d
Compare
78e608d
to
00942dd
Compare
@@ -24,7 +24,7 @@ macro-paths: ["macros"] | |||
|
|||
target-path: "../build" # directory which will store compiled SQL files | |||
log-path: "../logs" # directory which will store DBT logs | |||
packages-install-path: "/tmp/dbt_modules" # directory which will store external DBT dependencies | |||
packages-install-path: "/dbt-tmp/dbt_modules" # directory which will store external DBT dependencies |
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.
for simpler naming, it doesn't need to be related to tmp
, what about going directly with /dbt_modules
or even /dbt
(without the dbt_modules)?
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.
I like /dbt
too. It's not really tmp since these are files we want to keep and continually use.
Yes, you have description in the release process doc here too: https://docs.google.com/document/d/1Q1N4oomtz3rXqqn97nMB5rfcM_Dd2K3in5vmYPWdjjY/edit especially you need to run integration tests because the output Line 27 in 2d0cc78
so they need to be regenerated with your changes |
...zation/integration_tests/normalization_test_output/mssql/test_simple_streams/dbt_project.yml
Show resolved
Hide resolved
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! What Chris said about testing and publishing before we merge this in.
00942dd
to
a6d590b
Compare
@ChristopheDuong I tried to run normalization integration tests locally, but I wasn't able to get this working on my M1 laptop. I don't think I ever managed to get any of the Is there a workaround to generate those |
Actually ignore my last comment @ChristopheDuong , I asked Edward for M1 python help and we resolved the blocker I was running into, running the local integration tests now and will follow the release process guidelines now that I'm unblocked |
Sorry for the message spam @ChristopheDuong but I spoke too soon I think - normalization integration tests are failing for me, even if I run in master. I don't want to block this PR on my M1-specific woes so if you or @davinchia able to run the integration tests locally for this branch and commit the generated file changes, that'd be greatly appreciated! |
it seems like some .sql files for clickhouse/mssql were not up to date either, so it's now part of your PR. I also had to add a little change in |
Thanks so much Chris! |
/test connector=bases/base-normalization
|
I'm not yet sure what's going on with the integration tests, but I ran into the same error ( |
You can talk to @girarda, he ran into that weeks ago too and had to solve it already i think. IIRC this is because this PR has side effects on all other branches: #9610 |
/test connector=bases/base-normalization
|
/publish connector=bases/base-normalization
|
/publish connector=bases/base-normalization
|
@Phlair It looks like the last publish run failed in "Check if connector definition is in definitions.yaml" - I'm guessing this is because normalization isn't actually tracked in there? I.e. this PR should be ready to merge without further intervention? |
@edgao correct, good to merge, no auto-bump for normalization (yet). This makes me think the automated comments could be clearer since this shouldn't look like a fail. |
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.
@pmossman I added an entry to the changelog; if that looks right to you then we should be good to merge here!
Thanks so much @edgao for all your help getting this over the finish line! Changelog looks good, going to merge this now |
What
Addresses #11163
Context: in #10761, a kube volume mount at
/tmp
was added for connector pods that need to write temporary data to disk. This was reverted because kube acceptance tests were consistently failing after this change.After some digging, I found that normalization was failing due to an empty
/tmp/dbt_modules
directory, and this made sense as the new volume mount at/tmp
was clearly overwriting the compile-time dbt modules that were stored there.Slack thread with some context/convo: https://airbytehq-team.slack.com/archives/C02TXQ020QM/p1648763773988619
How
This PR updates our dbt project templates to store dbt_modules at
/dbt-tmp
instead of/tmp
so that the volume mount no longer conflicts. It also un-reverts the original PR to re-introduce the/tmp
volume mount.Misc Questions
base-normalization
? I'll need a/publish
command at some point right? (I haven't ever had to usepublish
so this is the first time I'm running into this process).