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

log 'depends_on_nodes' node attribute in the log record 'extra' field #2341

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

dcereijodo
Copy link
Contributor

@dcereijodo dcereijodo commented Apr 18, 2020

resolves #2316

Description

Changed logger configuration to log depends_on_nodes node attribute when logging with the ModelMetadata context.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot
Copy link

cla-bot bot commented Apr 18, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @dcereijodo

@drewbanin
Copy link
Contributor

Thanks for opening this issue @dcereijodo! Tagging @beckjake to review.

Before we get into a proper review though - do you mind resubmitting the CLA form? I see that the address entered into the form was only partially entered. We're going to need a complete mailing address in order to record the CLA submission to get this PR merged. Thanks for your help!

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

Hi @dcereijodo, this looks like a great change to me - thanks for contributing!

Can you please add links to this issue/PR in the changelog under Features similar to the other entries? For the text, something like "Add a 'depends_on_nodes' attribute to log record extra field" should be great.

Also please add your username and a link to this PR to the contributors list so we can make sure you get credit in the release notes.

@cla-bot
Copy link

cla-bot bot commented Apr 21, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @dcereijodo

@dcereijodo dcereijodo marked this pull request as ready for review April 21, 2020 06:29
@dcereijodo
Copy link
Contributor Author

Hey. I've updated the changelog accordingly.

I've also submitted again the CLA, but it seems that the bot complained again. It's just this form a need to fill in and submit no?

I anyways tried one more time. You can let me know here or in Slack if it's still not working.

Captura de pantalla 2020-04-21 a las 8 28 42

@beckjake
Copy link
Contributor

@dcereijodo Thank you! After you submit, there's a second manual step on our end - should be all set now.

@beckjake
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Apr 21, 2020
@cla-bot
Copy link

cla-bot bot commented Apr 21, 2020

The cla-bot has been summoned, and re-checked this pull request!

@drewbanin
Copy link
Contributor

hey @dcereijodo - based on your comment here (#2316 (comment)) this PR should be ready to go, right? If so, are you able to fix the merge conflict in the changelog? Once that's addressed, I think we'll be ready to ship this as-is

@dcereijodo
Copy link
Contributor Author

@drewbanin it should be fine now.
Thanks 😄!

@drewbanin drewbanin requested a review from beckjake April 29, 2020 12:28
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for your contribution @dcereijodo!

@beckjake beckjake merged commit 6708046 into dbt-labs:dev/octavius-catto Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log test node metadata: depends_on_nodes
3 participants