You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/14 16:24:20 UTC
[GitHub] [airflow] LydiaSimmons523 opened a new pull request, #24446: make exists() method on GoogleDriveHook return false for files that are trashed
LydiaSimmons523 opened a new pull request, #24446:
URL: https://github.com/apache/airflow/pull/24446
If a file is created in Google Drive and then moved to the Trash, the exists() method will return true for the file. This change makes the exists() method check whether the file is 'trashed' and returns false if so.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1155426064
Why not add `include_trashed` flag in the function signature and let each user decide what behavior is desired for the specific use case?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
mik-laj commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1155986542
Can you add unit tests to avoid regression?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24446:
URL: https://github.com/apache/airflow/pull/24446#discussion_r897556605
##########
airflow/providers/google/suite/hooks/drive.py:
##########
@@ -128,17 +128,19 @@ def get_media_request(self, file_id: str) -> HttpRequest:
request = service.files().get_media(fileId=file_id)
return request
- def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None):
+ def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None, include_trashed: Optional[bool] = False):
Review Comment:
Why Optional? I think this can simply take `bool`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1155421794
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1159814474
Tests were failing. Rebased to check if that was another problem. If they still fails @LydiaSimmons523 - please fix them and ping me
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1214301854
fixed in https://github.com/apache/airflow/pull/25675
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal closed pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
eladkal closed pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
URL: https://github.com/apache/airflow/pull/24446
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LydiaSimmons523 commented on a diff in pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
LydiaSimmons523 commented on code in PR #24446:
URL: https://github.com/apache/airflow/pull/24446#discussion_r897912427
##########
airflow/providers/google/suite/hooks/drive.py:
##########
@@ -128,17 +128,19 @@ def get_media_request(self, file_id: str) -> HttpRequest:
request = service.files().get_media(fileId=file_id)
return request
- def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None):
+ def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None, include_trashed: Optional[bool] = False):
Review Comment:
I think having it not consider the trashed files is a sensible default. Also, this will avoid breaking existing code that calls this method on the hook.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LydiaSimmons523 commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
LydiaSimmons523 commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1155439359
OK, I went ahead and did that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pankajastro commented on a diff in pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
pankajastro commented on code in PR #24446:
URL: https://github.com/apache/airflow/pull/24446#discussion_r898110720
##########
airflow/providers/google/suite/hooks/drive.py:
##########
@@ -172,12 +174,13 @@ def get_file_id(self, folder_id: str, file_name: str, drive_id: Optional[str] =
else:
files = (
service.files()
- .list(q=query, spaces="drive", fields="files(id, mimeType)", orderBy="modifiedTime desc")
+ .list(q=query, spaces="drive", fields="files(id, mimeType, trashed)", orderBy="modifiedTime desc")
.execute(num_retries=self.num_retries)
)
+ print(files)
Review Comment:
Please remove print statement
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1180339595
@LydiaSimmons523 are you still working on 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24446:
URL: https://github.com/apache/airflow/pull/24446#discussion_r901248118
##########
airflow/providers/google/suite/hooks/drive.py:
##########
@@ -128,17 +128,19 @@ def get_media_request(self, file_id: str) -> HttpRequest:
request = service.files().get_media(fileId=file_id)
return request
- def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None):
+ def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None, include_trashed: Optional[bool] = False):
Review Comment:
That explains `= False`, but not `Optional`. This can be achieved with `include_trashed: bool = False`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1159966000
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] KSchmidAmilar commented on a diff in pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
KSchmidAmilar commented on code in PR #24446:
URL: https://github.com/apache/airflow/pull/24446#discussion_r897910650
##########
airflow/providers/google/suite/hooks/drive.py:
##########
@@ -128,17 +128,19 @@ def get_media_request(self, file_id: str) -> HttpRequest:
request = service.files().get_media(fileId=file_id)
return request
- def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None):
+ def exists(self, folder_id: str, file_name: str, drive_id: Optional[str] = None, include_trashed: Optional[bool] = False):
Review Comment:
I think not including the trashed files is a sensible default. Keeping it optional will also avoid breaking existing code that uses this function on the hook.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24446: make exists() method on GoogleDriveHook return false for files that are trashed
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24446:
URL: https://github.com/apache/airflow/pull/24446#issuecomment-1159832155
Seems like you have failing tests to fix.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org