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

Move pytest-logbook dependency to dev_requirements.txt #2505

Merged
merged 3 commits into from
Jun 4, 2020

Conversation

aburgel
Copy link
Contributor

@aburgel aburgel commented May 30, 2020

Description

pytest-logbook is only used for testing dbt, so I don't think it should be a dependency of dbt-core. I've moved it to dev_requirements.txt so it should still be available for tests.

I came across this because my project has a dependency on dbt-core and also uses pytest. pytest's caplog fixture stopped working when we upgraded dbt to 0.15.3. I was able to trace this back to pytest-logbook which installs its own log capture fixture and disables (breaks?) the builtin one.

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 May 30, 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: @aburgel

1 similar comment
@cla-bot
Copy link

cla-bot bot commented May 30, 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: @aburgel

@cla-bot cla-bot bot added the cla:yes label May 31, 2020
@aburgel aburgel changed the base branch from dev/marian-anderson to dev/octavius-catto May 31, 2020 19:12
@drewbanin drewbanin removed the cla:yes label Jun 1, 2020
@beckjake
Copy link
Contributor

beckjake commented Jun 1, 2020

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jun 1, 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: @aburgel

@cla-bot
Copy link

cla-bot bot commented Jun 1, 2020

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

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.

Hey @aburgel, thanks for your contribution! I'm not sure why the cla-bot accepted your PR initially, but we'll definitely need to have you sign that before we can merge this. I'm very interested in getting this fix in to 0.17.0 and cutting a new RC in the immediate future, so please let me know if you're having any trouble on that front!

@aburgel
Copy link
Contributor Author

aburgel commented Jun 1, 2020

@beckjake Working on it now. Just confirming with my company which document I need to sign.

@aburgel
Copy link
Contributor Author

aburgel commented Jun 4, 2020

@beckjake Just submitted the CLA!

@beckjake
Copy link
Contributor

beckjake commented Jun 4, 2020

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jun 4, 2020

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

@cla-bot cla-bot bot added the cla:yes label Jun 4, 2020
@beckjake
Copy link
Contributor

beckjake commented Jun 4, 2020

Thanks @aburgel! Looks good, but I have some bad news - we already released rc4, which we really really hope is the last RC.

I think I'm fine with merging this in for the 0.17.0 release since it appears very low-risk and actually bit me a couple times on my current work - @drewbanin what's your take on this?

Assuming we do decide we want this for 0.17.0, it would be great if you could pull in/rebase onto current dev/octavius-catto and update your changelog entry to be at the top, instead of where it is now (I think it'll show as under rc4 after merge).

@aburgel
Copy link
Contributor Author

aburgel commented Jun 4, 2020

@beckjake just rebased and moved the changelog entry. my bad for not getting the CLA signed sooner, took a bit for our legal team to review.

@drewbanin
Copy link
Contributor

@beckjake I'm ok with merging this for 0.17.0-final - can you work to kick off the tests again and get this merged?

Thanks for your contribution @aburgel :)

@beckjake beckjake merged commit 2636363 into dbt-labs:dev/octavius-catto Jun 4, 2020
@beckjake
Copy link
Contributor

beckjake commented Jun 4, 2020

All merged, this will make it into 0.17.0 - thanks again!

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.

3 participants