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

Use correct paths in GcsBucket.put/get_directory #14432

Merged
merged 5 commits into from
Jul 5, 2024

Conversation

rooperuu
Copy link
Contributor

@rooperuu rooperuu commented Jul 1, 2024

closes #12487

Fixes target path resolution in GcsBucket methods. Currently, in put_directory, the bucket_folder prefix is added twice, and in get_directory, the prefix is not removed.

Example

GCS target file for put_directory given local source file from_path/file.txt:

  • Before: bucket_folder/bucket_folder/to_path/file.txt
  • After: bucket_folder/to_path/file.txt

Local target file for get_directory given GCS source file bucket_folder/from_path/file.txt:

  • Before: to_path/from_path/file.txt
  • After: to_path/file.txt

Checklist

  • This pull request includes a label categorizing the change e.g. maintenance, fix, feature, enhancement, docs.
  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

hi @rooperuu ! thank you for the PR

this generally looks good to me, but 2 questions:

  • why switch away from self.write_path here?
  • can you add a test for the new path creation logic?

@zzstoatzz zzstoatzz mentioned this pull request Jul 2, 2024
5 tasks
@rooperuu
Copy link
Contributor Author

rooperuu commented Jul 2, 2024

Thanks for taking a look! I have updated the tests for get_directory to ensure that correct paths are used. Testing put_directory is a bit more tricky unless the return value can be changed, as it currently only returns the number of files.

Regarding the other point, the problem was that path resolution was done both in put_directory and in write_path, so one or the other should be eliminated. This approach is more similar to the one in get_directory.

@aaazzam
Copy link
Collaborator

aaazzam commented Jul 3, 2024

Thanks @rooperuu for adding those tests. @zzstoatzz does this LGTU? Would love to get this merged / released since this is blocking for this team. 🙇

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thank you @rooperuu !

@zzstoatzz zzstoatzz merged commit eb367af into PrefectHQ:main Jul 5, 2024
12 checks passed
zzstoatzz added a commit that referenced this pull request Jul 10, 2024
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.

prefect deployment with gcs-bucket storage block loading into bucket incorrectly
3 participants