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. |
1 similar comment
|
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. |
dbeatty10
left a comment
dbeatty10
left a comment
There was a problem hiding this comment.
@VersusFacit The branch for the PR has conflicts to be resolved. Could you resolve those?
gshank
left a comment
gshank
left a comment
There was a problem hiding this comment.
This PR needs to have main merged because there have been a lot of changes in the events code and the messages as implemented in types.proto here will no longer work.
Rather than a bunch of new logging events I would prefer to have them combined as much as makes sense. We are going through an exercise of reducing the number of events and this is going in the opposite direction.
VersusFacit
force-pushed
the
CT-738/dbt_debug_log_fix
branch
from
9a8c470 to
f0c5f5b
Compare
|
|
||
| class TestProfiles: | ||
| def dbt_debug(self, project_dir_cli_arg=None, profiles_dir_cli_arg=None): | ||
| def dbt_debug(self, project, project_dir_cli_arg=None, profiles_dir_cli_arg=None): |
There was a problem hiding this comment.
Excited about all the simplification you were able to do for the tests/functional/profiles/test_profile_dir.py test!
Is adding project essential? If not, what do you think about leaving it as the following?
| def dbt_debug(self, project, project_dir_cli_arg=None, profiles_dir_cli_arg=None): | |
| def dbt_debug(self, project_dir_cli_arg=None, profiles_dir_cli_arg=None): |
There was a problem hiding this comment.
@dbeatty10 It is because under the hood, the dbt invocation needs the profile for the run command; otherwise run_dbt or run_dbt_and_capture won't work -- without this I run into "profile not found errors". Kept barking at me during development.
You have a better solution by chance?
There was a problem hiding this comment.
Yikes, I was wrong. This was needed at some point, but now I can go in and remove it. Thanks for the callout 🙇♀️ 😅 I need to dive in the next time I find the error I describe, so I can identify if it's a context-specific bug or something more general. I've run into it too many times to be misremembering that, but also, my understanding of it has now been shifted as a result of this convo.
tl;dr merged your change
There was a problem hiding this comment.
No worries!
Will be a very happy day once this is merged and my custom run_dbt_and_capture_stdout is no longer a thing!
Will re-review shortly.
dbeatty10
left a comment
dbeatty10
left a comment
There was a problem hiding this comment.
I only reviewed the changes to tests/functional/profiles/test_profile_dir.py, and they look great!
Relying on other reviewers to cover everything else.
dbeatty10
dismissed
their stale review
Behavior was confirmed and suggestion was applied
4 participants
resolves #5353
And what a fun, instructive excursion this was. I got to edit some CLI jobs and walk around some execution parts of the repo -- how exciting 😊
What does this PR do?
dbt debugstructured loggingdbt --log-format json debugandDBT_LOG_FORMAT=json dbt debugboth now render as structured logsprintleft in the file -- sweet!dbt {ls,list}structured loggingdbt --log json {ls,list}andDBT_LOG_FORMAT=json dbt {ls,list}prints structured logs now, underinfoleveldbt {ls,list}continues to print just the names (🔪 -ed out some overrides and aTODOor two)Before and After Screenshots
My debugging strategy:
Check evolving structured logs against plain ol'
dbt debug.Before, several printed lines:
After, they're all structured logs now:
Minor caveat: These were taken in two different iterations so the connections listed are slightly different, but I just have lots of profiles :p
Outstanding Questions
protoc -I. --python_betterproto_out=. types.proto. Hope that's right 🙇♀️Checklist
changie newto create a changelog entry