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.

Sign up for GitHub

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

Jump to bottom

Break out credentials as a separate module #1391

Merged
merged 9 commits into from
Nov 5, 2024
Merged

Break out credentials as a separate module #1391

merged 9 commits into from
Nov 5, 2024

Conversation

mikealfare
Copy link
Contributor

Problem

This supports the retries fix. We need to reference BigQueryCredentials in multiple places. The current structure would create circular references.

Solution

Put BigQueryCredentials in its own module. Move over other dependent objects as needed. Take this time to also fix all the imports. The last change became necessary due to already existing import issues that were uncovered by making this a new module.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@mikealfare mikealfare self-assigned this Oct 31, 2024
@mikealfare mikealfare requested a review from a team as a code owner October 31, 2024 20:59
@cla-bot cla-bot bot added the cla:yes label Oct 31, 2024
@github-actions GitHub Actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-bigquery contributing guide.

@mikealfare mikealfare added the Skip Changelog Skips GHA to check for changelog file label Oct 31, 2024
mikealfare
mikealfare commented Nov 1, 2024
View reviewed changes
Copy link
Contributor Author

@mikealfare mikealfare left a comment

Choose a reason for hiding this comment

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

There are a lot of things moving around, but nothing actually changed. All changes are one of:

  • moved an object from .connections to .credentials
  • fixed import order
  • imported object directly from the module in which it's defined to avoid circular imports
  • updated imports due to an object moving from .connections to .credentials
  • updated mocks in unit tests due to an object moving from .connections to .credentials

dbt/adapters/bigquery/__init__.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed, moved BigQueryCredentials to .credentials.

dbt/adapters/bigquery/column.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed.

dbt/adapters/bigquery/dataproc/batch.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed, moved DataprocBatchConfig to .credentials.

dbt/adapters/bigquery/dataset.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed.

dbt/adapters/bigquery/gcloud.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed.

dbt/adapters/bigquery/relation.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same imports, order fixed.

tests/unit/test_bigquery_adapter.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved get_bigquery_defaults to .credentials.

tests/unit/test_configure_dataproc_batch.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved get_bigquery_defaults to .credentials.

dbt/adapters/bigquery/credentials.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved BigQueryCredentials, Priority, BigqueryConnectionMethod, DataprocBatchConfig, and get_bigquery_defaults from .connections to .credentials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved gcloud contents here to avoid circular import.

dbt/adapters/bigquery/connections.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved BigQueryCredentials, Priority, BigqueryConnectionMethod, DataprocBatchConfig, and get_bigquery_defaults from .connections to .credentials.
Imported __version__ directly instead of from .bigquery to avoid circular import.
Fixed import order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved gcloud into credentials to avoid circular import.

@mikealfare mikealfare enabled auto-merge (squash) November 1, 2024 19:48
mikealfare
mikealfare commented Nov 1, 2024
View reviewed changes
dbt/adapters/bigquery/gcloud.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into credentials to avoid circular import.

@mikealfare mikealfare removed the Skip Changelog Skips GHA to check for changelog file label Nov 4, 2024
@mikealfare mikealfare merged commit 35c32f1 into main Nov 5, 2024
42 checks passed
@mikealfare mikealfare deleted the break-out-credentials-module branch November 5, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-rogers-dbt colin-rogers-dbt colin-rogers-dbt approved these changes

Assignees

@mikealfare mikealfare

Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikealfare @colin-rogers-dbt