-
Notifications
You must be signed in to change notification settings - Fork 26
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(dbt sync): Merge metadata and preserve Preset configs #238
Conversation
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 key in ("changed_on", "created_on", "type_generic"): | ||
if key in metadata: | ||
del metadata[key] | ||
|
||
return metadata |
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 key in ("changed_on", "created_on", "type_generic"): | |
if key in metadata: | |
del metadata[key] | |
return metadata | |
return { | |
k: v for k, v in metadata.items() | |
if k not in {"changed_on", "created_on", "type_generic"} | |
} |
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.
(You might also want to make this function more generic by passing the keys that should be filtered out as an argument.)
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.
that's a great idea! I think for now the scope might be limited, but I'll check if this is used in other parts of the code (not necessarily related with the dbt sync) and I can improve this in a future PR
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.
Unsure why, but the suggested change breaks our current tests. It might require updating the tests as well. I'll try to cover this too while simplifying sync_datasets
.
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
By default, when triggering a dbt sync the CLI:
While this is the expected scenario for the integration (to preserve dbt as the source of truth), this approach can be impactful for organizations that are already extensively using Preset/Superset and therefore have custom metrics defined in their datasets that don't exist in dbt yet. For these cases, the sync could result in broken charts/dashboards, since metrics that only exist in Preset don't get re-created.
This PR introduces two modifications:
--preserve-columns
flag to--preserve-metadata
: The--preserve-columns
was used to prevent a column sync during the update, to properly preserve column configs (Is temporal, Is dimension, etc). This flag is now re-named to--preserve-metadata
and also:--merge-metadata
: This flag can be used to:The validation if the metric already exists in Preset happens comparing the metric
name
from dbt with themetric_name
from Preset.