-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore(deps): update google-cloud-tasks dependencies #13037
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request, authored by glasnt, aims to update the google-cloud-tasks
dependency across several examples and update the test to use dynamic queue names. The intent is to address issue #12917 (though the description doesn't elaborate).
Here's a breakdown of the changes:
- Dependency Updates: The
google-cloud-tasks
dependency is updated to version 2.17.1 across multiplerequirements.txt
files. This affects the following examples:appengine/flexible/tasks/create_app_engine_queue_task_test.py
appengine/flexible/tasks/requirements.txt
appengine/flexible_python37_and_earlier/tasks/requirements.txt
cloud_tasks/http_queues/requirements.txt
cloud_tasks/snippets/requirements.txt
- Test Improvement: The
appengine/flexible/tasks/create_app_engine_queue_task_test.py
file is modified. Specifically, lines 21-36 were added to ensure a unique queue name is used for each test run, preventing conflicts. The test now also cleans up by deleting the created queue after the test completes.
The changes appear straightforward and well-intentioned. I'll be checking for any potential regressions or side effects during my full review.
Here's a little haiku to brighten your day:
Code flows like a stream,
Tests run green, a joyful sight,
Bugs flee in the night.
Best,
Gemini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request updates the google-cloud-tasks
dependency to version 2.17.1 and introduces dynamic queue names for tests in create_app_engine_queue_task_test.py
. This addresses issue #12917 and improves the reliability of the tests by preventing conflicts between test runs. The changes are well-implemented and straightforward.
|
||
import create_app_engine_queue_task | ||
|
||
TEST_PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT") | ||
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", "my-appengine-queue") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to explicitly set the random.seed
for UUID generation to ensure test reproducibility. While the current implementation using uuid.uuid4()
is generally sufficient, setting a seed provides stronger guarantees, especially in CI environments.
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") | |
import random | |
random.seed(0) # Set a seed for UUID generation |
|
||
import create_app_engine_queue_task | ||
|
||
TEST_PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT") | ||
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", "my-appengine-queue") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") | ||
|
||
|
||
def test_create_task(): | ||
result = create_app_engine_queue_task.create_task( | ||
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION | ||
) | ||
assert TEST_QUEUE_NAME in result.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a try...finally
block to ensure the queue is deleted even if the assertion fails. This prevents orphaned queues from accumulating during test failures.
assert TEST_QUEUE_NAME in result.name | |
try: | |
assert TEST_QUEUE_NAME in result.name | |
finally: | |
# delete created queue | |
from google.cloud import tasks_v2 | |
client = tasks_v2.CloudTasksClient() | |
client.delete_queue( | |
tasks_v2.DeleteQueueRequest(name=client.queue_path(TEST_PROJECT_ID, TEST_LOCATION, TEST_QUEUE_NAME)) | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request, authored by glasnt, aims to update the google-cloud-tasks
dependency across several sample applications and address issue #12917. The key changes include:
- Dependency Update: The
google-cloud-tasks
dependency is updated to version 2.17.1 across multiplerequirements.txt
files. This is reflected in the following files:appengine/flexible/tasks/requirements.txt
appengine/flexible_python37_and_earlier/tasks/requirements.txt
cloud_tasks/http_queues/requirements.txt
cloud_tasks/snippets/requirements.txt
- Test Improvement: The test file
appengine/flexible/tasks/create_app_engine_queue_task_test.py
is modified to include cleanup of dynamically created queues after each test run. This ensures that test runs don't leave behind resources. Specifically, lines 31-36 were added to delete the queue after the test completes.
The changes are straightforward and appear to be well-tested. I'll be looking for any potential conflicts or unintended side effects during my full review.
And now, a little haiku to lighten the mood:
Update the tasks,
Dependencies now aligned,
Code flows smoothly.
Thanks,
Gemini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request updates the google-cloud-tasks
dependency to 2.17.1 and introduces dynamic queue names for tests in create_app_engine_queue_task_test.py
. This addresses issue #12917 and improves the reliability of the tests by preventing conflicts between test runs. The changes are well-implemented and straightforward.
|
||
import create_app_engine_queue_task | ||
|
||
TEST_PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT") | ||
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", "my-appengine-queue") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to include a short explanation of why the UUID is being used in the queue name. This helps with readability and understanding the purpose of this change.
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") | |
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") # Use UUID to make queue name unique |
result = create_app_engine_queue_task.create_task( | ||
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a try...finally
block to ensure the queue is deleted even if the assertion or the create_task
call fails. This prevents orphaned queues from accumulating.
result = create_app_engine_queue_task.create_task( | |
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION | |
) | |
try: | |
result = create_app_engine_queue_task.create_task( | |
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION | |
) | |
assert TEST_QUEUE_NAME in result.name | |
finally: | |
# delete created queue | |
from google.cloud import tasks_v2 | |
client = tasks_v2.CloudTasksClient() | |
try: | |
client.delete_queue(name=client.queue_path(TEST_PROJECT_ID, TEST_LOCATION, TEST_QUEUE_NAME)) | |
except google.api_core.exceptions.NotFound: | |
print('Queue already deleted') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:
This pull request, "chore(deps): update google-cloud-tasks dependency, dynamic queues", aims to update the google-cloud-tasks
dependency across several files within the python-docs-samples
repository. This addresses issue #12917.
Here's a breakdown of the changes:
- Dependency Updates: The
google-cloud-tasks
dependency is updated from various versions (2.13.1, 2.14.2) to version 2.17.1 across multiplerequirements.txt
files. This includes updates in:appengine/flexible/tasks/requirements.txt
appengine/flexible_python37_and_earlier/tasks/requirements.txt
cloud_tasks/http_queues/requirements.txt
cloud_tasks/snippets/requirements.txt
- Test File Modification: The file
appengine/flexible/tasks/create_app_engine_queue_task_test.py
is modified to include code that deletes the created queue after the test completes. This ensures that test queues are cleaned up, preventing resource leaks. Specifically, lines 31-36 were added to handle queue deletion using thegoogle.cloud.tasks_v2
library.
The changes appear straightforward and focused on updating dependencies and improving test cleanup. I'll provide a more detailed review shortly.
And now, a little haiku to brighten your day:
Code flows like a stream,
Updates refresh, clean and bright,
Bugs flee in the night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request updates the google-cloud-tasks dependency to 2.17.1 and makes a necessary change to create dynamic queue names in the tests. This is a good change that prevents conflicts between test runs and makes the tests more robust. The changes are straightforward and well-implemented.
|
||
import create_app_engine_queue_task | ||
|
||
TEST_PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT") | ||
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") | ||
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", "my-appengine-queue") | ||
TEST_QUEUE_NAME = f"my-appengine-queue-{uuid.uuid4()}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a dynamically generated queue name is a good approach for tests to avoid conflicts. However, the queue should be deleted after the test completes to avoid accumulating unused queues. Wrap the test in a try/finally block to ensure the queue is deleted even if the test fails.
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}")
try:
result = create_app_engine_queue_task.create_task(
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION
)
assert TEST_QUEUE_NAME in result.name
finally:
# delete created queue
from google.cloud import tasks_v2
client = tasks_v2.CloudTasksClient()
client.delete_queue(
tasks_v2.DeleteQueueRequest(name=client.queue_path(TEST_PROJECT_ID, TEST_LOCATION, TEST_QUEUE_NAME))
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:
This pull request, authored by glasnt, aims to update the google-cloud-tasks
dependency across several files and address issue #12917. The core change is upgrading the dependency to version 2.17.1 from various older versions (2.13.1, 2.14.2).
Here's a breakdown of the changes:
- Dependency Updates: The
google-cloud-tasks
dependency has been updated to version2.17.1
in the following files:appengine/flexible/tasks/requirements.txt
appengine/flexible_python37_and_earlier/tasks/requirements.txt
cloud_tasks/http_queues/requirements.txt
cloud_tasks/snippets/requirements.txt
- Test File Modification: The test file
appengine/flexible/tasks/create_app_engine_queue_task_test.py
has been modified. Specifically, lines 13-44 were altered to include the creation and subsequent deletion of a test queue using thegoogle.cloud.tasks_v2
library. This ensures that the tests are self-contained and don't rely on pre-existing queues. The queue name is now dynamically generated usinguuid.uuid4()
to prevent naming conflicts.
The changes appear to be straightforward and well-documented. I'll be looking for any potential conflicts or regressions during my full review.
And now, a little haiku to lighten the mood:
Code flows like a stream,
Updates refresh, bugs take flight,
A cleaner, new build.
Thanks,
Gemini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request updates the google-cloud-tasks dependency and implements dynamic queue names for tests, which is a positive change. The addition of the queue deletion logic within the test function ensures better resource management and prevents orphaned queues. However, the suggested code in the original response had some issues, including an unnecessary try block in the first file and an incorrect placement of the try/finally block in the second file. Also, the library update should be reflected in the requirements files.
|
||
import create_app_engine_queue_task | ||
from google.cloud import tasks_v2 | ||
|
||
TEST_PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT") | ||
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamically generated queue name is a good approach to avoid conflicts between test runs. The cleanup step to delete the queue after the test is also a good practice. The try block is unnecessary here as NotFoundError is already being handled in the except block.
TEST_LOCATION = os.getenv("TEST_QUEUE_LOCATION", "us-central1") | |
TEST_QUEUE_NAME = os.getenv("TEST_QUEUE_NAME", f"my-appengine-queue-{uuid.uuid4()}") | |
def test_create_task(): | |
result = create_app_engine_queue_task.create_task( | |
TEST_PROJECT_ID, TEST_QUEUE_NAME, TEST_LOCATION | |
) | |
assert TEST_QUEUE_NAME in result.name | |
# delete created queue | |
from google.cloud import tasks_v2 | |
client = tasks_v2.CloudTasksClient() | |
try: | |
client.delete_queue( | |
tasks_v2.DeleteQueueRequest(name=client.queue_path(TEST_PROJECT_ID, TEST_LOCATION, TEST_QUEUE_NAME)) | |
) | |
except NotFound as e: | |
print(f"Ignoring NotFound, details: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide a full review shortly. In the meantime, here's a quick summary for you and other reviewers to get up to speed:
This pull request, authored by glasnt, aims to update the google-cloud-tasks
dependency across several files and address issue #12917. The primary change is upgrading the google-cloud-tasks
library to version 2.17.1 from various older versions (2.13.1, 2.14.2). This update is reflected in the following files:
appengine/flexible/tasks/create_app_engine_queue_task_test.py
: This file's test suite was modified. Specifically, lines 13-44 were altered to include the creation and deletion of a dynamically named queue using thegoogle.cloud.tasks_v2
library. This ensures that tests don't interfere with each other by using unique queue names.appengine/flexible/tasks/requirements.txt
appengine/flexible_python37_and_earlier/tasks/requirements.txt
cloud_tasks/http_queues/requirements.txt
cloud_tasks/snippets/requirements.txt
These files all received updates to theirgoogle-cloud-tasks
dependency version to 2.17.1.
The changes appear straightforward and well-documented. I'll be checking for any potential compatibility issues or regressions during my full review.
Here's a little haiku to brighten your day:
Code flows like a stream,
Updates refresh, bugs take flight,
Clean code takes the stage.
Thanks,
Gemini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request updates the google-cloud-tasks
dependency to 2.17.1 and introduces dynamic queue names for tests in create_app_engine_queue_task_test.py
. This addresses issue #12917 and improves the reliability of the tests by preventing conflicts between test runs. The changes are well-implemented and straightforward.
Description
Fixes #12917