-
Notifications
You must be signed in to change notification settings - Fork 0
Add update_path_tags function for managing S3 object tags #37
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 89.39% 89.45% +0.05%
==========================================
Files 37 37
Lines 3489 3508 +19
Branches 518 525 +7
==========================================
+ Hits 3119 3138 +19
Misses 251 251
Partials 119 119
🚀 New features to boost your workflow:
|
src/aibs_informatics_aws_utils/s3.py
Outdated
| There are three modes for updating tags: | ||
| - replace: Replace all existing tags with new tags | ||
| - append: Merge new tags with existing tags | ||
| - delete: Delete specified tags from existing tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth clarifying that this would delete the specified tag keys regardless of what the value provided with the tag key is. i.e. it doesn't have to match the value of the tag key in the object.
Clarify that values do not matter when deleting tags.
| s3_paths = list_s3_paths(s3_path=s3_path, **kwargs) | ||
| logger.info(f"Updating tags for {len(s3_paths)} objects under prefix {s3_path}") | ||
| for nested_s3_path in s3_paths: | ||
| update_path_tags(nested_s3_path, tags, mode, recursive=False, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Loop is probably more "pythonic" compared to recursion
You could create a s3_objects_to_tag: list[S3URI] that gets populated by list_s3_paths if recursive=True otherwise s3_objects_to_tag would just be a 1-element list if s3_path is_object. Then just loop over L617-643.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the recursion vs loop is probably not where things are held up. We could try to make this multithreaded because a lot of it is just waiting for network responses
njmei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit but otherwise LGTM
What's in this Change?
This change adds a new function for s3 path tagging
update_path_tags. this allows for tagging objects at or prefixed by a given s3 path.Testing
adding unit tests