-
Notifications
You must be signed in to change notification settings - Fork 28
🎨 replace project_id and node_id with appropriate labels when exporting #7508
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
base: master
Are you sure you want to change the base?
🎨 replace project_id and node_id with appropriate labels when exporting #7508
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7508 +/- ##
===========================================
- Coverage 87.43% 69.00% -18.44%
===========================================
Files 1740 739 -1001
Lines 67389 34628 -32761
Branches 1143 170 -973
===========================================
- Hits 58924 23895 -35029
- Misses 8145 10675 +2530
+ Partials 320 58 -262
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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.
just wondering why you did not re-use the function that is already around?
).get_project_id_and_node_id_to_names_map( | ||
project_uuids=_get_project_ids(user_selecton={x[0] for x in source_object_keys}) | ||
) | ||
|
||
archive_entries: ArchiveEntries = [ |
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.
what about re-using this??
def update_display_fields(self, id_name_mapping: dict[str, str]) -> None:
display_path = f"{self.path}"
for old, new in id_name_mapping.items():
display_path = display_path.replace(old, urllib.parse.quote(new, safe=""))
self.display_path = Path(display_path)
instead of _replace_node_id_project_id_in_path
?
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.
What did not convince me about this is:
- I require different escaping here
- does not run in linear time
What I like about it:
- only replaces what it finds (no risk of not finding anything)
The actual think that annoys me is the non liner runtime. If a user exports one or more projects, the more nodes there are the worst this gets.
This will scale with the number of files times the number of nodes + projects instead of just the number of files
services/storage/src/simcore_service_storage/utils/simcore_s3_dsm_utils.py
Outdated
Show resolved
Hide resolved
…-project-and-node-in-exports
|
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.
as discussed. thanks!
What do these changes do?
When exporting data,
project_id
is replaced with the project's name whilenode_id
is replaced with the node's label.Also, special character
/
is escaped with_
.Related issue/s
How to test
Dev-ops checklist