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

Wider google-cloud dependencies #2877

Merged
merged 7 commits into from
Nov 23, 2020
Merged

Wider google-cloud dependencies #2877

merged 7 commits into from
Nov 23, 2020

Conversation

max-sixty
Copy link
Contributor

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

  • 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 Nov 10, 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: @max-sixty

@cla-bot
Copy link

cla-bot bot commented Nov 10, 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: @max-sixty

@max-sixty
Copy link
Contributor Author

Re CLA — I will get back to you on that, though not immediately

@max-sixty
Copy link
Contributor Author

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 BIGQUERY_SERVICE_ACCOUNT_JSON, but I'm now getting BIGQUERY_TEST_ALT_DATABASE errors. The only reference I can see is in test.env which is gitignore-d

@jtcohen6
Copy link
Contributor

Hey @max-sixty, thanks for contributing!

  • It looks like tests for this are running in your "max-sixty" CircleCI org. Is that tied to your GitHub account? Normally, tests on all PRs (including forks) automatically run in the Fishtown CircleCi org, with all the right env vars. Check out this comment for a workaround.
  • What's your timeline for signing the CLA? This is a change I'd like to include in the next release, which should be a matter of weeks

@cla-bot
Copy link

cla-bot bot commented Nov 10, 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: @max-sixty

@max-sixty
Copy link
Contributor Author

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

@max-sixty
Copy link
Contributor Author

Looks like tests pass!

Shall I pin the upper end of the Google Cloud dependencies to the currently release "minor" version (i.e. major.minor.patch)?

While semver should allow us to pin to the current major version, as @drewbanin states, Google has let some breaking changes into minor versions:

We probably do not want to set the upper bound to <2.0.0 because breaking changes in libraries like this one can (and do) break older versions of dbt down the line.

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.

@max-sixty
Copy link
Contributor Author

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 dbt-bigquery dependency as the bigquery plugin inside this repo rather than the bigquery plugin in PyPI?

@max-sixty
Copy link
Contributor Author

max-sixty commented Nov 10, 2020

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

@max-sixty
Copy link
Contributor Author

Unfortunately I now get an error when running dbt debug. What's the standard way of testing out changes in plugins?

Configuration:
  profiles.yml file [ERROR invalid]
  dbt_project.yml file [OK found and valid]

Profile loading failed for the following reason:
Runtime Error
  Credentials in profile "{foo}", target "user" invalid: Runtime Error
    Could not find adapter type bigquery!

@cla-bot
Copy link

cla-bot bot commented Nov 11, 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: @max-sixty

@jtcohen6
Copy link
Contributor

Hey @max-sixty, thanks for the quick work here, and sorry for the delayed response —

While semver should allow us to pin to the current major version, as @drewbanin states, Google has let some breaking changes into minor versions:

We probably do not want to set the upper bound to <2.0.0 because breaking changes in libraries like this one can (and do) break older versions of dbt down the line.

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 1.22.4 google-api-core, since 1.23.0 was just released a few weeks ago—but we should then set an upper bound against the next minor version (<1.23.0).

how can we get pip to view the dbt-bigquery dependency as the bigquery plugin inside this repo rather than the bigquery plugin in PyPI?

My thought would be to either:

  • Install from pip install https://github.com/max-sixty/dbt/archive/unlock-google-api-deps.zip
  • Mock the local development process: clone the repository and pip install -r editable_requirements.txt

@max-sixty
Copy link
Contributor Author

My thought would be to either:

* Install from `pip install https://github.com/max-sixty/dbt/archive/unlock-google-api-deps.zip`

* Mock the local development process: clone the repository and `pip install -r editable_requirements.txt`

Right — I think the latter isn't possible to specify to pip / poetry — it would require manually downloading and installing.
I'll continue doing the first — I'm sure you have good reasons for organizing the repo this way — though it does make this sort of testing harder.

I also couldn't seem to use dbt-utils because the version number was 0.19 — so I reset the version number back to 0.18.1. Let me know if I'm making a mistake there

@cla-bot
Copy link

cla-bot bot commented Nov 11, 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: @max-sixty

@max-sixty
Copy link
Contributor Author

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 1.22.4 google-api-core, since 1.23.0 was just released a few weeks ago—but we should then set an upper bound against the next minor version (<1.23.0).

Done — I think you meant <1.24.0, lmk if not

@jtcohen6
Copy link
Contributor

I also couldn't seem to use dbt-utils because the version number was 0.19 — so I reset the version number back to 0.18.1. Let me know if I'm making a mistake there

That's a defensive upper bound in dbt-utils, in case the next minor dbt version has breaking changes. You can use the --no-version-check flag for this.

Done — I think you meant <1.24.0, lmk if not

I was thinking we'd stick to <1.23.0 since 1.23.0 just came out, but I don't really have strong feelings either way. <1.24.0 is fine if the tests pass.

@max-sixty
Copy link
Contributor Author

That's a defensive upper bound in dbt-utils, in case the next minor dbt version has breaking changes. You can use the --no-version-check flag for this.

Great, thanks

@max-sixty
Copy link
Contributor Author

I think I have now signed the CLA — does this not show up @drewbanin ?

@jtcohen6
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Nov 19, 2020
@cla-bot
Copy link

cla-bot bot commented Nov 19, 2020

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

@jtcohen6
Copy link
Contributor

@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

@max-sixty
Copy link
Contributor Author

Done!

@jtcohen6
Copy link
Contributor

@max-sixty thanks for the contribution!

@jtcohen6 jtcohen6 merged commit 0951d08 into dbt-labs:dev/kiyoshi-kuromiya Nov 23, 2020
@max-sixty max-sixty deleted the unlock-google-api branch November 23, 2020 19:31
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.

[BigQuery] Allow more recent versions of google-api-core?
2 participants