Skip to content

Conversation

rhpvorderman
Copy link
Contributor

Motivation

...
#640

Approach

Simply add the flag 😉.
There is also one bug fix about an invalid cross-device link that gets triggered because I have my /home dir on a separate partition. So /tmp and /home are on different devices and this triggers an invalid cross-device link warning when os.rename is used. I use the higher level shutil.move instead. This bug fix is in a single commit that can be cherry picked.

I have not added tests yet because you might have different ideas about how to implement this. I think a "--license" flag to include a license file that also gets added to the manifest would be quite a neat feature for redistributable WDL packages.

Checklist

  • Add appropriate test(s) to the automatic suite
  • Use make pretty to reformat the code with black
  • Use make check to statically check the code using Pyre and Pylint
  • Send PR from a dedicated branch without unrelated edits
  • Ensure compatibility with this project's MIT license

@coveralls
Copy link

coveralls commented May 7, 2023

Pull Request Test Coverage Report for Build 4924706285

  • 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 95.189%

Changes Missing Coverage Covered Lines Changed/Added Lines %
WDL/Zip.py 7 8 87.5%
Files with Coverage Reduction New Missed Lines %
WDL/runtime/task.py 1 95.84%
Totals Coverage Status
Change from base Build 4776833032: 0.004%
Covered Lines: 7320
Relevant Lines: 7690

💛 - Coveralls

@mlin
Copy link
Collaborator

mlin commented May 7, 2023

Thanks @rhpvorderman, this definitely looks useful. Only suggestion is to error out if an additional file will cause something else to be overwritten.

I've hit the problem with os.rename too, so appreciate the fix for that!

@rhpvorderman
Copy link
Contributor Author

Thanks @rhpvorderman, this definitely looks useful. Only suggestion is to error out if an additional file will cause something else to be overwritten.

Done. Luckily python already has a FileExistsError class built in, so picking the right error was made easy.

@rhpvorderman rhpvorderman force-pushed the additionalfilesinzip branch from 0315e3f to 6b76b80 Compare May 8, 2023 13:09
@mlin mlin merged commit 46e700d into chanzuckerberg:main May 9, 2023
@rhpvorderman
Copy link
Contributor Author

Thanks!

@rhpvorderman rhpvorderman deleted the additionalfilesinzip branch May 10, 2023 11:10
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.

3 participants