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.

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

Add drive download utility functions and test #462

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Apophenia
Copy link
Contributor

@Apophenia Apophenia commented Nov 27, 2024

  • Adds utility functions for use in moving drive files to s3

from processes.util.google_integration import get_drive_file

def test_get_drive_file():
test_id = os.environ['EXAMPLE_FILE_ID']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using a test file ID in my local-compose. Could we upload a small file to test with, assuming the UUID of a drive file is otherwise anonymous (e.g. doesn't expose anything else about our account info or structure)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely!

@Apophenia Apophenia marked this pull request as ready for review November 27, 2024 15:35
Comment on lines 39 to 43





Copy link
Contributor

Choose a reason for hiding this comment

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

nit: only need one new line at the end of the file!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, thanks, vs code was no longer linting on save!

logger.warning(f"HTTP error occurred when downloading Drive file {file_id}: {error}")
file = None
except Exception as err:
logger.warning(f"Unexpected {type(err)=} occurred when downloading drive file {file_id}: {error}")
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.exception works well in this case to capture the error type and message

return build('drive', 'v3', credentials=credentials)

def get_drive_file(file_id: str) -> BytesIO:
drive_service = create_drive_service(service_account_info=json.loads(SERVICE_ACCOUNT_FILE))
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the use of functions rather than a class here. Could we create the drive_service at the top level of this file so it's used more like a singleton?


except HttpError as error:
logger.warning(f"HTTP error occurred when downloading Drive file {file_id}: {error}")
file = None
Copy link
Contributor

Choose a reason for hiding this comment

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

we can return early here and just have return None

downloader = MediaIoBaseDownload(file, request)

done = False
while done is False:
Copy link
Contributor

Choose a reason for hiding this comment

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

while not done I think may read better here!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting this in the services folder instead is more explicit - e.g. services/google_drive_service.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.

yeah, I think it's probably better this to refactor this into a service structure and move it there

Copy link
Contributor

@kylevillegas93 kylevillegas93 left a comment

Choose a reason for hiding this comment

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

Overall, looks good - nice work! just a few minor comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants