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

Feature/cleanup tar ball #1300

Merged
merged 7 commits into from
Apr 14, 2021
Merged

Conversation

izcram
Copy link

@izcram izcram commented Apr 6, 2021

addressing #1292 by introducing a tarball_files_keep_list.txt file enumerating which files should be added in the final tarball. Additionally also add a tarball_files_discard_list.txt file to be able to warn when an unaccounted file in the release dir is found (which could mean, that it was added in a pipeline iteration, but was forgotten to be added to the tarball)

Furthermore, changed output of the md5sum generation step to be compatible with GNU md5sum. This way, file integrity can be checked using md5sum -c md5sums.txt

@melissacline
Copy link
Contributor

The files to discard list looks great, and that's going to make such a difference!
In files to keep, there are a couple that maybe we don't need to keep.

  • In the artifacts directory, e have both merged.tsv and aggregated.tsv. Would you remind me of the difference between those? I suspect we only need one of the two.
  • Would you also remind me of the difference between built_with_change_types.tsv and built_final.tsv? I apologize that I should remember this, since we discussed it only a couple weeks ago... built_final.tsv has no change type field. Is there any other difference? If not, these seem redundant, and we should pick one of the two.

Thanks!

@izcram
Copy link
Author

izcram commented Apr 9, 2021

@melissacline

  1. yes indeed! fixed in c7088e6
  2. in 05e8f92 I changed the symlink of built_final.tsv to point to built_with_change_types.tsv mainly to make it clear that this is the final output. So it is redundant, but doesn't use more space :) I adjusted the top_level_readme.txt accordingly

Copy link
Member

@zfisch zfisch left a comment

Choose a reason for hiding this comment

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

this is great!

@zfisch zfisch merged commit 8234892 into BRCAChallenge:master Apr 14, 2021
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