search

gouline / dbt-metabase

dbt + Metabase integration
https://pypi.org/project/dbt-metabase/
MIT License
448 stars 64 forks source link

Fix sync when models are listed under dbt sources #24

Closed remigabillet closed 3 years ago

remigabillet commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

remigabillet commented 3 years ago

Sounds good. Great job on wrapping up #19 !

remigabillet commented 3 years ago

I'm planning to working on the rebase today.

remigabillet commented 3 years ago

I closed this one and replaced it with #27