Adds "disable config" sample to manage_transfer_configs.py #13578
Conversation
Adds a Python sample to disable a BQ DTS transfer configuration.
|
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
the
samples
Summary of ChangesHello @kevinwchin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Python sample to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new sample for disabling a BigQuery DTS transfer configuration. The implementation is straightforward, but it's missing a corresponding test case, which is a critical omission for ensuring the sample works correctly. I've also provided a couple of suggestions to improve the code's robustness and readability by addressing a common Python pitfall and improving the clarity of a client library call. Please add the test and consider the other suggestions.
| def disable_config(override_values={}): | ||
| # [START bigquerydatatransfer_disable_config] | ||
| from google.cloud import bigquery_datatransfer | ||
| from google.protobuf import field_mask_pb2 | ||
|
|
||
| transfer_client = bigquery_datatransfer.DataTransferServiceClient() | ||
|
|
||
| transfer_config_name = "projects/1234/locations/us/transferConfigs/abcd" | ||
| # [END bigquerydatatransfer_disable_config] | ||
| # To facilitate testing, we replace values with alternatives | ||
| # provided by the testing harness. | ||
| transfer_config_name = override_values.get( | ||
| "transfer_config_name", transfer_config_name | ||
| ) | ||
| # [START bigquerydatatransfer_disable_config] | ||
|
|
||
| transfer_config = bigquery_datatransfer.TransferConfig(name=transfer_config_name) | ||
| transfer_config.disabled = True | ||
|
|
||
| transfer_config = transfer_client.update_transfer_config( | ||
| { | ||
| "transfer_config": transfer_config, | ||
| "update_mask": field_mask_pb2.FieldMask(paths=["disabled"]), | ||
| } | ||
| ) | ||
|
|
||
| print(f"Updated config: '{transfer_config.name}'") | ||
| print(f"Is config disabled: '{transfer_config.disabled}'") | ||
| # [END bigquerydatatransfer_disable_config] | ||
| # Return the config name for testing purposes, so that it can be deleted. | ||
| return transfer_config |
There was a problem hiding this comment.
The newly added disable_config function is missing a corresponding unit test in manage_transfer_configs_test.py. Adding a test is essential to verify the functionality of this sample and prevent future regressions. Please add a test that calls this function and asserts that the transfer config is disabled correctly.
| # Return the config name for testing purposes, so that it can be deleted. | ||
| return transfer_config | ||
|
|
||
| def disable_config(override_values={}): |
There was a problem hiding this comment.
Using a mutable object like a dictionary as a default argument can lead to unexpected behavior if it's ever modified, as the modification will persist across calls. It's safer to use None as the default.1
After applying this suggestion, please add the following lines at the beginning of the function body:
if override_values is None:
override_values = {}This pattern is used throughout the file and should ideally be corrected everywhere for consistency and robustness.
| def disable_config(override_values={}): | |
| def disable_config(override_values=None): |
Style Guide References
Footnotes
-
Avoid using mutable default arguments in function definitions. Instead, use a default value of None and initialize the mutable type within the function. This prevents hard-to-debug issues where a default object is modified and that modification persists across function calls. ↩
| transfer_config = transfer_client.update_transfer_config( | ||
| { | ||
| "transfer_config": transfer_config, | ||
| "update_mask": field_mask_pb2.FieldMask(paths=["disabled"]), | ||
| } | ||
| ) |
There was a problem hiding this comment.
For better readability, consider using keyword arguments when calling update_transfer_config instead of passing a dictionary. This makes the call more explicit and easier to understand at a glance.
transfer_config = transfer_client.update_transfer_config(
transfer_config=transfer_config,
update_mask=field_mask_pb2.FieldMask(paths=["disabled"]),
)
Description
Adds a Python sample to disable a BQ DTS transfer configuration. We're missing a sample here in https://cloud.google.com/bigquery/docs/working-with-transfers#disable_a_transfer
Checklist