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

fix(bigquery): missing dependency #4567

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

anshbansal
Copy link
Collaborator

@anshbansal anshbansal commented Apr 4, 2022

#3782 introduced a regression due to missing dependencies
#4561 fixed usage sources but missed bigquery

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.

@@ -150,7 +150,7 @@ def get_long_description():
# PyAthena is pinned with exact version because we use private method in PyAthena
"athena": sql_common | {"PyAthena[SQLAlchemy]==2.4.1"},
"azure-ad": set(),
"bigquery": sql_common | bigquery_common | {"sqlalchemy-bigquery>=1.4.1"},
"bigquery": sql_common | bigquery_common | usage_common | {"sqlalchemy-bigquery>=1.4.1"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a bit misleading we name it as usage_common and we use it with non-usage sources

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add the sqlparser directly?
my concern is that from now on all the additional usage_common dependencies will automatically be a dependency of the bigquery source as well.
Which will be fine if we merge usage with source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

Unit Test Results (metadata ingestion)

       5 files  ±0         5 suites  ±0   54m 10s ⏱️ - 1m 23s
   394 tests ±0     394 ✔️ ±0    0 💤 ±0  0 ±0 
1 814 runs  ±0  1 776 ✔️  - 7  38 💤 +7  0 ±0 

Results for commit 50662d9. ± Comparison against base commit 57ea768.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

Unit Test Results (build & test)

  96 files    96 suites   17m 18s ⏱️
686 tests 627 ✔️ 59 💤 0

Results for commit 50662d9.

@gabe-lyons gabe-lyons merged commit 1478489 into datahub-project:master Apr 4, 2022
@anshbansal anshbansal deleted the fix-dependencies branch April 4, 2022 15:37
maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
* fix(bigquery): missing dependency

* code review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants