-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Wider google-cloud dependencies #2877
Wider google-cloud dependencies #2877
Conversation
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: @max-sixty |
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: @max-sixty |
Re CLA — I will get back to you on that, though not immediately |
Are there docs on how to run integration tests in CircleCI? I've created a dummy project and put the service account json in the |
Hey @max-sixty, thanks for contributing!
|
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: @max-sixty |
Great — looks like disabling my CircleCI works, and it's now building under the main project's: https://app.circleci.com/pipelines/github/fishtown-analytics/dbt/1511/workflows/36fbd065-613e-4409-86f5-cf6092f232ef/jobs/33037 I'm not sure re CLA — I've escalated internally |
Looks like tests pass! Shall I pin the upper end of the Google Cloud dependencies to the currently release "minor" version (i.e. While semver should allow us to pin to the current major version, as @drewbanin states, Google has let some breaking changes into minor versions:
The alternative would be something like what we do in xarray, where we always test on the latest version of dependencies, and then fix any issues as they arise. |
So we can test this on our workflows; let me know if you know off hand how to install this with pip — how can we get pip to view the |
This is my current version for testing this out — it would be great to have a cleaner way of including updated dependencies, but not sure how to navigate pip / setuptools into allowing it from a subpackage, given they're currently defined as separate packages rather than sub-packages: https://github.com/max-sixty/dbt/compare/unlock-google-api...max-sixty:unlock-google-api-deps |
Unfortunately I now get an error when running
|
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: @max-sixty |
Hey @max-sixty, thanks for the quick work here, and sorry for the delayed response —
I'd rather stick to Drew's original rationale and be defensive here. I'm happy with testing against the latest stable minor version—I'd say that's
My thought would be to either:
|
Right — I think the latter isn't possible to specify to pip / poetry — it would require manually downloading and installing. I also couldn't seem to use |
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: @max-sixty |
Done — I think you meant |
That's a defensive upper bound in dbt-utils, in case the next minor dbt version has breaking changes. You can use the
I was thinking we'd stick to |
Great, thanks |
I think I have now signed the CLA — does this not show up @drewbanin ? |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
@max-sixty Thanks for the CLA follow-up. Could you add a changelog note under v0.19.0 + add yourself to the list of contributors there? Then I think this is good to ship |
Done! |
@max-sixty thanks for the contribution! |
resolves #2794
Description
As discussed in the issue. I can narrow the range of these as per that issue; figured I'd ensure tests pass before fixing the versions.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.