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

Fix sync when models are listed under dbt sources #24

Closed
wants to merge 0 commits into from

Conversation

remigabillet
Copy link
Contributor

#17 introduced a bug by collecting models listed under DBT sources.

The problem is that sources' tables are typically not stored under the DBT schema but this script only collected Metabase models under the DB schema specified as an argument.
In consequence, when a "source" model is loaded, the script won't be able to match it with a Metabase model and the script times out.

The solution implemented makes the script "schema-aware" by collecting the schema name explicitly for sources. It also indexes all models from Metabase in order to be able to find models outside the DBT Schema.

The behavior for DBT models outside sources is unchanged. They are assume to be stored under the "schema" passed explicitly as an argument.

This change is a stepping stone towards addressing #3

@remigabillet remigabillet changed the title Remi/fix sources Fix sync when models are listed under DBT Sources Jun 25, 2021
@remigabillet
Copy link
Contributor Author

remigabillet commented Jun 25, 2021

@z3z1ma Does this make sense ?
I think you're fixing this issue in #19 but I think master is broken right now, so this could be a quick fix.

@gouline gouline changed the title Fix sync when models are listed under DBT Sources Fix sync when models are listed under dbt Sources Jun 28, 2021
@gouline gouline changed the title Fix sync when models are listed under dbt Sources Fix sync when models are listed under dbt sources Jun 28, 2021
@gouline
Copy link
Owner

gouline commented Jun 28, 2021

I'll leave it to @z3z1ma to review this pull request, since it's based on his change.

However, @remigabillet please fix any mention of dbt to the proper capitalisation "dbt" (not "DBT").

@z3z1ma
Copy link
Contributor

z3z1ma commented Jun 29, 2021

Hello @remigabillet

Let me review this in the evening. Thanks for your contribution here! That last PR was a big push to transition to reading the manifest json artifact which implicitly solves for the schema issue for models and sources alike.

So because this change is focused on what is now dubbed the folder parser, we just need to make sure these changes sit on top of the codebase in the new PR so we might need to either rebase or re integrate your code on top of the new code after we review it.

@remigabillet
Copy link
Contributor Author

Thanks for having a look @z3z1ma. Since master is currently broken, and assuming this PR is satisfying, I suggest we merge it in first.
With that said, #19 is a much bigger deal so I'll let you determine what you think is the best order of operation.

@gouline
Copy link
Owner

gouline commented Jun 30, 2021

PR #19 is now merged, please rebase your changes @remigabillet. Once done, @z3z1ma and I can review.

@remigabillet
Copy link
Contributor Author

Sounds good. Great job on wrapping up #19 !

@remigabillet
Copy link
Contributor Author

I'm planning to working on the rebase today.

@remigabillet
Copy link
Contributor Author

I closed this one and replaced it with #27

@gouline gouline added this to the v0.8.x milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants