Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fixing empty logging in list_folders and list_blobs #214

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Fixing empty logging in list_folders and list_blobs #214

merged 2 commits into from
Sep 28, 2023

Conversation

bjarneschroeder
Copy link
Contributor

@bjarneschroeder bjarneschroeder commented Sep 28, 2023

Fix: Log of list_folders and list_blobs now displays bucket name and path

Closes #184.

When calling list_folders or list_blobs on a GcsBucket the log message always displayed bucket_path, a result of _join_bucket_folder.
If we are in the root directory however, _join_bucket_folder returns None.
If we hand a value for folder we receive the joined path.

Changes made

Depending on wether _join_bucket_folder returns None, which would mean we are listing from the root directory, we conditionally log a message containing the current bucket_path or just self.bucket.

I did not write tests for the log messages. If its needed, I will happily do so.

This will have to be discussed and I am happy to receive some guidance on this:
There exists a warning in the doc of _join_bucket_folder to be careful to not call it twice in nested method calls.
With my implementation it is now called with the folder parameter in list_folders and list_blobs.
Since bucket_path is not used in any other way in list_folders we should be fine.
However, I see that this might lead to some bug down the road.
Thats why I wrote a warning comment.

This seemed to be the best approach to me, since other methods would have involved more conditional checking of self.bucket_folder/folder in list_folders and combining them in some way in the message,
which seemed to be too bloated just for a more sophisticated log message.
I also thought about _resolve_path, but it would lead to the same dangerous problem down the road and _join_bucket_folder has a condition to check, if the method got called twice.

If you prefer any of the mentioned methods or have a different idea, I am happy to implement it.

Example

When running this code:

from prefect_gcp import GcsBucket

bucket = GcsBucket(
    bucket="my-bucket",
)

bucket.list_folders()

The log output is:

08:35:21.362 | INFO    | prefect.GcsBucket - Listing folders in bucket 'my-bucket'.
08:35:21.677 | INFO    | prefect.GcsBucket - Listing blobs in bucket 'my-bucket'.

When running this code:

from prefect_gcp import GcsBucket

bucket = GcsBucket(
    bucket="my-bucket",
    bucket_folder="my-folder"
)

bucket.list_folders()

The log output is:

08:37:58.936 | INFO    | prefect.GcsBucket - Listing folders in 'my-folder' in bucket 'my-bucket'.
08:37:59.287 | INFO    | prefect.GcsBucket - Listing blobs in folder 'my-folder' in bucket 'my-bucket'.

When running test_list_folders_with_subfolders the log output is:

08:40:48.379 | INFO | prefect.GcsBucket - Listing folders in 'base_folder/sub_folder' in bucket 'bucket'.

08:40:48.382 | INFO | prefect.GcsBucket - Listing blobs in folder 'base_folder/sub_folder' in bucket 'bucket'.

Checklist

  • References any related issue by including "Closes #" or "Closes ".
    • If no issue exists and your change is not a small fix, please create an issue first.
  • Includes tests or only affects documentation.
  • Passes pre-commit checks.
    • Run pre-commit install && pre-commit run --all locally for formatting and linting.
  • Includes screenshots of documentation updates.
    • Run mkdocs serve view documentation locally.
  • Summarizes PR's changes in CHANGELOG.md

@bjarneschroeder bjarneschroeder requested a review from a team as a code owner September 28, 2023 06:49
@bjarneschroeder
Copy link
Contributor Author

This is my first ever in an open-source project. So forgive me the question: Do I check the boxes or do you? 😄

Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution! 🎉

@desertaxle desertaxle merged commit 78fbac0 into PrefectHQ:main Sep 28, 2023
10 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list_blobs and list_folder log incorrect bucket name
2 participants