-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add secondary_profiles to profile.py
#11308
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your patch status has failed because the patch coverage (42.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11308 +/- ##
=======================================
Coverage 88.95% 88.96%
=======================================
Files 189 189
Lines 24169 24182 +13
=======================================
+ Hits 21500 21513 +13
Misses 2669 2669
Flags with carried forward coverage won't be shown. Click here to find out more.
|
profile.py
Converted to draft temporarily to see if I can address feedback from @amychen1776 re making |
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.
I left a few minor nits and one readability/maintainability suggestion. As far as interfaces go, this aligns with what the base adapter will expect (basically secondary_profiles
).
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.
Approved from Adapters' point of view.
# if we only have one target, we can infer the target name | ||
# currently, this is only used for secondary profiles | ||
target_name = next(iter(raw_profile["outputs"])) | ||
fire_event(MissingProfileTarget(profile_name=profile_name, target_name=target_name)) |
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.
Why is this event necessary in this case?
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.
The event name is slightly misleading, but the message is useful for debugging purposes to indicate that we inferred a target_name
:
dbt-core/core/dbt/events/types.py
Lines 68 to 73 in e60b41d
class MissingProfileTarget(InfoLevel): | |
def code(self) -> str: | |
return "A005" | |
def message(self) -> str: | |
return f"target not specified in profile '{self.profile_name}', using '{self.target_name}'" |
I'll add a comment for the same.
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.
Added a comment here, let me know if any other changes are needed here.
Resolves XPLAT-241.
Problem
We need to parse
secondary_profiles
associated with a given profile.Solution
secondary_profiles
dict on theProfile
class.target_name
to be optional iff a secondary profile has a single target.Checklist