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(dbt): Platform instances for target platform #5129

Merged
merged 20 commits into from Jun 12, 2022

Conversation

skrydal
Copy link
Contributor

@skrydal skrydal commented Jun 9, 2022

Adds 2 configuration fields target_platform_instance and target_platform_instance_mapping to allow for setting target platform instance values.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

Unit Test Results (build & test)

381 tests  ±0   381 ✔️ ±0   2m 57s ⏱️ -37s
  89 suites ±0       0 💤 ±0 
  89 files   ±0       0 ±0 

Results for commit ac5d513. ± Comparison against base commit 1b50709.

♻️ This comment has been updated with latest results.

@@ -456,11 +462,27 @@ def get_custom_properties(node: DBTNode) -> Dict[str, str]:
return custom_properties


def match_target_platform_instance(
Copy link
Contributor

Choose a reason for hiding this comment

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

lets call this get_entity_platform_instance, since it may be getting a platform instance for a dbt node and not a target node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

self.config.platform_instance,
match_target_platform_instance(
self.config.target_platform_instance,
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

why set this as none here? let's let the logic of the method handle whether to use the dbt platform instance or not. that will make things less error prone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I added reference to config platform_instance here. Originally here there was a None if I recall correctly and I wanted to preserve the original logic, so that in case platform would match dbt it would still return None as before. But if you think we should return platform instance I adjusted it to return platform instance.

@github-actions
Copy link

github-actions bot commented Jun 10, 2022

Unit Test Results (metadata ingestion)

       5 files         5 suites   1h 35m 33s ⏱️
   555 tests    552 ✔️   3 💤 0
2 552 runs  2 477 ✔️ 75 💤 0

Results for commit ac5d513.

♻️ This comment has been updated with latest results.

@skrydal skrydal requested a review from gabe-lyons June 10, 2022 08:22
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM! smoke test failures seem unrelated.

@shirshanka shirshanka merged commit 503208b into datahub-project:master Jun 12, 2022
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