-
Notifications
You must be signed in to change notification settings - Fork 11
Feat: Accept multiple maven zips with non-RADAS signing way #350
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
ligangty
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.
@yma96 sorry for late review. Please see comments.
charon/pkgs/maven.py
Outdated
| src_file = os.path.join(root_dir, file) | ||
| dest_file = os.path.join(dest_root, file) | ||
| if os.path.exists(dest_file): | ||
| overwritten_count += 1 |
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 don't think we should do this overwritten copy. Instead we can just mark it as "duplicated"
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 just think one case: if has two same files name, but content may not be consistent, we will always keep the latter coming one as the latest to override, @ligangty WDYT?
I also thought of the checksum method, but it might cost much for the thousands of comparisons, but such situations that require override are rare.
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.
No need to do this, because we don't know which one is the right one. If there is overlapping, let's always consider the first one as the right one.
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.
ligangty
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.
Ok. Let's merge this PR and see the test result to see if any issues.
No description provided.