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

feat: Add flag to skip syncing sources, and allow meta-defined display names #95

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

atvaccaro
Copy link
Contributor

This tool is amazing!

I have a couple features that may be helpful.

First, adding a flag to skip all dbt sources. In our project, we ultimately use Metabase on top of BigQuery, but we don't import source datasets into Metabase. Without the flag, dbt-metabase throws errors as it tries to sync dbt source models.

Second, adding metabase.display_name as an optional field. We deal with a lot of GTFS data, so our model names naturally have GTFS as a common acronym. Currently it gets title-cased as Gtfs in many situations, which is annoying. So I wanted to have the ability to override the Metabase display_name property.

Copy link
Owner

@gouline gouline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run CI checks locally and fix any failures before pushing, GitHub waits for me to run them in Actions for first-time contributors.

@z3z1ma Do you remember what source models are used for after reading, apart from being exported to Metabase? If they are not exported, is there any point in reading them in the first place? Trying to think whether they should be excluded from the Metabase client (how this PR implements it) or from being read in the parsers.

dbtmetabase/logger/logging.py Show resolved Hide resolved
dbtmetabase/__init__.py Show resolved Hide resolved
@atvaccaro
Copy link
Contributor Author

Run CI checks locally and fix any failures before pushing, GitHub waits for me to run them in Actions for first-time contributors.

Ah I'm sorry! To be honest I didn't notice them until I got the failure email, but will do that.

Re: formatting, I don't see a black version pinned. Would you be open to pinning 22.3.0?

@z3z1ma
Copy link
Contributor

z3z1ma commented Apr 13, 2022

@atvaccaro

black is in requirements-test.txt but it isn't pinned. No deps are pinned currently actually, though the latest compatible versions would be chosen on fresh install.

@gouline
dbt-metabase models only uses source for exporting definitions.
dbt-metabase exposures uses sources as possible matches for BI exposure extraction

I think it might be cleaner/more logical to exclude in the parser with a flag and only pass that flag from the models command when the CLI flag exclude_sources is passed

@gouline
Copy link
Owner

gouline commented Apr 13, 2022

Re: formatting, I don't see a black version pinned. Would you be open to pinning 22.3.0?

No regular work is done on this repository so nobody will be updating those pinned versions, use latest. Create a virtual env if it conflicts with other projects.

@gouline
Copy link
Owner

gouline commented Jul 5, 2022

@atvaccaro Still planning to get this merged?

@gouline gouline changed the title Add flag to skip syncing sources, and allow meta-defined display names feat: Add flag to skip syncing sources, and allow meta-defined display names Jul 6, 2022
@gouline gouline changed the base branch from master to feat/exclude-sources July 6, 2022 10:07
@gouline gouline deleted the branch gouline:feat/exclude-sources July 6, 2022 10:08
@gouline gouline closed this Jul 6, 2022
@gouline gouline reopened this Jul 6, 2022
@gouline gouline force-pushed the feat/exclude-sources branch from 647e5cd to 7d9fbbf Compare July 6, 2022 10:14
@gouline gouline merged commit d83c9f6 into gouline:feat/exclude-sources Jul 6, 2022
gouline added a commit that referenced this pull request Jul 6, 2022
* feat: Add flag to skip syncing sources, and allow meta-defined display names (#95)

* handle display names defined in meta, and replace description newlines with spaces since metabase throws them away

* allow flag to toggle metabase sources sync; default is to skip

* Fixes for code review and CI errors

* Parser test for model display name

Co-authored-by: Andrew Vaccaro <[email protected]>
gouline added a commit that referenced this pull request Jul 7, 2022
* Raise style fix

* feat: Allow excluding dbt sources when exporting to Metabase (#109)

* feat: Add flag to skip syncing sources, and allow meta-defined display names (#95)

* handle display names defined in meta, and replace description newlines with spaces since metabase throws them away

* allow flag to toggle metabase sources sync; default is to skip

* Fixes for code review and CI errors

* Parser test for model display name

Co-authored-by: Andrew Vaccaro <[email protected]>

* fix: circular foreign key reference (#110)

* fix: circular foreign key reference (#106)

Co-authored-by: Mike Gouline <[email protected]>

* Fmt

* Nicer Python

Co-authored-by: Joël Luijmes <[email protected]>

* Add display_name for columns

* Quote fix

* Add to test

Co-authored-by: Mike Gouline <[email protected]>
Co-authored-by: Andrew Vaccaro <[email protected]>
Co-authored-by: Joël Luijmes <[email protected]>
gouline added a commit that referenced this pull request Jul 7, 2022
* Add `display_name` option for field names (#111)

* Raise style fix

* feat: Allow excluding dbt sources when exporting to Metabase (#109)

* feat: Add flag to skip syncing sources, and allow meta-defined display names (#95)

* handle display names defined in meta, and replace description newlines with spaces since metabase throws them away

* allow flag to toggle metabase sources sync; default is to skip

* Fixes for code review and CI errors

* Parser test for model display name

Co-authored-by: Andrew Vaccaro <[email protected]>

* fix: circular foreign key reference (#110)

* fix: circular foreign key reference (#106)

Co-authored-by: Mike Gouline <[email protected]>

* Fmt

* Nicer Python

Co-authored-by: Joël Luijmes <[email protected]>

* Add display_name for columns

* Quote fix

* Add to test

Co-authored-by: Mike Gouline <[email protected]>
Co-authored-by: Andrew Vaccaro <[email protected]>
Co-authored-by: Joël Luijmes <[email protected]>

* Display name implementation and tests

Co-authored-by: Jack Cook <[email protected]>
Co-authored-by: Andrew Vaccaro <[email protected]>
Co-authored-by: Joël Luijmes <[email protected]>
@atvaccaro
Copy link
Contributor Author

@atvaccaro Still planning to get this merged?

Hey I'm so sorry and thank you for getting this through. I've been heads-down on a major project and refactor and never got back to this.

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.

3 participants