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

[18.09] Keep xlsx files compressed #6867

Merged
merged 2 commits into from Oct 15, 2018

Conversation

@mvdbeek
Copy link
Member

mvdbeek commented Oct 13, 2018

Fixes #6849

@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Oct 13, 2018

LGTM, should we have a unit test for this?

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Oct 13, 2018

@nsoranzo I started one earlier today, I will finish it and push to @mvdbeek branch

@mvdbeek

This comment has been minimized.

Copy link
Member Author

mvdbeek commented Oct 14, 2018

I think it'd be nice to test the shipped test data in an integration test case with and without auto_decompress, but I'd target dev with that.

@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Oct 14, 2018

I think it'd be nice to test the shipped test data in an integration test case with and without auto_decompress, but I'd target dev with that.

Agreed, but for 18.09 we could just add a unit test to verify that xlsx are sniffed correctly.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

mvdbeek commented Oct 14, 2018

It's not that I don't want to add a unit test for sniffing, but it wouldn't have caught the issue here, since the sniffing worked fine before, unless that would be a more intricate unit test that tests the upload_util module.

@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Oct 14, 2018

Isn't #6849 about xlsx sniffed as blastxml ?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

mvdbeek commented Oct 14, 2018

Yes, but it's not the sniffer's fault, if you have a datatype that is not marked as compressed the first file in the archive will be unpacked and sniffed, which in the case of xlsx files is a xml manifest.

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Oct 15, 2018

@mvdbeek The manifest should still not be sniffed as blastxml given the process described by @peterjc. However given the work in #6869 I think unit tests is not getting us much here and we are pretty unlikely to touch this in release branch I guess.

@martenson martenson merged commit 03073ed into galaxyproject:release_18.09 Oct 15, 2018
4 of 6 checks passed
4 of 6 checks passed
api test Build finished. 432 tests run, 1 skipped, 3 failed.
Details
framework test Build finished. 187 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
integration test Build finished. 123 tests run, 10 skipped, 0 failed.
Details
selenium test Build finished. 146 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@nsoranzo nsoranzo deleted the mvdbeek:fix_xlsx_unpacking branch Oct 15, 2018
@dannon dannon added this to the 18.09 milestone Oct 15, 2018
@galaxyproject galaxyproject deleted a comment from galaxybot Oct 15, 2018
@mvdbeek mvdbeek mentioned this pull request Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.