Re: [PR] Change gdrive system tests [airflow]
shahar1 commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3280179853 ## providers/google/src/airflow/providers/google/suite/transfers/gcs_to_gdrive.py: ## @@ -88,6 +88,7 @@ class GCSToGoogleDriveOperator(BaseOperator): "source_object", "destination_object", "impersonation_chain", +"destination_folder_id", Review Comment: Apologies that I wasn't clear, but I meant the PR title. The PR title currently mentions "Change gdrive system tests", while it does other stuff as well. Adding to the field description is unnecessary (however - adding a description to the PR would be very useful for reviewers). ## providers/google/tests/system/google/cloud/gcs/example_gdrive_to_gcs.py: ## Review Comment: It should be a part of this PR - adding `drive_id` without fixing `FOLDER_ID = ""` silently drops the folder filter in the hook, so the test searches the entire shared drive by name. In a system test environment the shared drive might be shared across test runs or users, meaning stale files with the same name could make the hook return the first match from the API in arbitrary order, causing the test to detect or download the wrong file. ## providers/google/tests/system/google/cloud/gcs/example_gdrive_to_gcs.py: ## @@ -73,8 +77,15 @@ start_date=datetime(2021, 1, 1), catchup=False, tags=["example", "gcs", "gdrive"], +render_template_as_native_obj=True, Review Comment: `render_template_as_native_obj=True` is for returning non-string types (dicts, ints) from templates - not needed here since `get_shared_drive_id()` returns a plain string; you could use destination_folder_id=get_shared_drive_id_task.output instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
Crowiant commented on PR #66930: URL: https://github.com/apache/airflow/pull/66930#issuecomment-4506552399 Hello @shahar1 Can we proceed with this PR if everything is ok? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
potiuk commented on PR #66930: URL: https://github.com/apache/airflow/pull/66930#issuecomment-4476940981 @Crowiant — There are 1 unresolved review thread on this PR from @phanikumv. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
Crowiant commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3257593179 ## providers/google/tests/system/google/cloud/gcs/example_gdrive_to_gcs.py: ## Review Comment: I understand that it could be misleading. But there is no default for this parameter in the operator. Could it be not the part of this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
Crowiant commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3257577627 ## providers/google/src/airflow/providers/google/suite/transfers/gcs_to_gdrive.py: ## @@ -88,6 +88,7 @@ class GCSToGoogleDriveOperator(BaseOperator): "source_object", "destination_object", "impersonation_chain", +"destination_folder_id", Review Comment: Added (templated) into the field description -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
Crowiant commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3257572242 ## providers/google/tests/system/google/cloud/gcs/example_gcs_to_gdrive.py: ## @@ -28,6 +28,7 @@ import logging import os from datetime import datetime +from functools import cache Review Comment: Hello @phanikumv! Thank you for your attention! Removed it from file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
choo121600 commented on PR #66930: URL: https://github.com/apache/airflow/pull/66930#issuecomment-4460969160 @Crowiant — Your unresolved review thread(s) from @phanikumv, @shahar1 appear to have been addressed (post-review commits and/or in-thread replies on every thread, with the latest commit pushed after the most recent thread). I've added the `ready for maintainer review` label so the PR re-enters the maintainer review queue. @phanikumv, @shahar1 — could you take another look when you have a chance? If you agree the feedback was addressed, please mark the threads as resolved so the queue signal stays accurate. If a thread still needs work, please reply in-line — @Crowiant will follow up. --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ --- Drafted-by: Claude Code (Opus 4.7); reviewed by @choo121600 before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
shahar1 commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3243103424 ## providers/google/src/airflow/providers/google/suite/transfers/gcs_to_gdrive.py: ## @@ -88,6 +88,7 @@ class GCSToGoogleDriveOperator(BaseOperator): "source_object", "destination_object", "impersonation_chain", +"destination_folder_id", Review Comment: Adding this as template field should be mentioned in the title -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
shahar1 commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3243103424 ## providers/google/src/airflow/providers/google/suite/transfers/gcs_to_gdrive.py: ## @@ -88,6 +88,7 @@ class GCSToGoogleDriveOperator(BaseOperator): "source_object", "destination_object", "impersonation_chain", +"destination_folder_id", Review Comment: Should be mentioned in the title -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
phanikumv commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3241827631 ## providers/google/tests/system/google/cloud/gcs/example_gdrive_to_gcs.py: ## Review Comment: FOLDER_ID = "" left in example_gdrive_to_gcs.py:63 and still passed as folder_id=FOLDER_ID to GoogleDriveFileExistenceSensor (~line 117) and GoogleDriveToGCSOperator (~line 127). With drive_id now supplied, an empty folder_id is at best confusing — likely should also resolve to the shared-drive ID. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Change gdrive system tests [airflow]
phanikumv commented on code in PR #66930: URL: https://github.com/apache/airflow/pull/66930#discussion_r3241785373 ## providers/google/tests/system/google/cloud/gcs/example_gcs_to_gdrive.py: ## @@ -28,6 +28,7 @@ import logging import os from datetime import datetime +from functools import cache Review Comment: Unused `from functools import cache` in both example Dags (example_gcs_to_gdrive.py:31, example_gdrive_to_gcs.py:24). Ruff F401 will block this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
