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

Fix extracting compressed downloaded files #2856

Merged
merged 1 commit into from Mar 21, 2020

Conversation

@AdamWill
Copy link
Contributor

AdamWill commented Mar 20, 2020

This regressed in #2736. When we want to download a compressed
file and extract it to produce the 'final' asset, we'll be doing
something like downloading 'http://example.com/file.img.gz' to
an asset called 'file.img'. After we download the asset, we write
it to a temporary file and feed that temporary file into
Archive::Extract, then extract it to the final asset location.
Archive::Extract uses the filename of the input file to determine
its type, so we should base the name of the temporary file on the
path component of the URL (where the compression is indicated) -
'file.img.gz' in this case - and not on the final target filename
(where the compression is not indicated) - 'file.img' in this
case. Before #2736, that's what the code did, but in #2736 it was
changed so the temporary filename is based on the target file.
I guess it was hard for @kraih to work this out without a real-
world example to understand how it's actually used in practice
(I think only Fedora uses this feature).

This returns the code to something similar to how it was before,
but using path()->to_string as @kraih's version did. It also
tweaks the test cases, which were clearly the wrong way round
once you know how this is supposed to work (it doesn't make
sense to expect extracting 'test.gz' to 'test.gz' to succeed,
but extracting 'test.gz' to 'test' to fail).

Signed-off-by: Adam Williamson awilliam@redhat.com

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Mar 20, 2020

@kraih @okurz @Martchus @perlpunk

Note I've tested the code change really works on our real instance, but the test changes I just did in my head, we'll see if CI passes :P

@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Mar 20, 2020

For reference, here's an example job that uses this feature in real life. The relevant settings are:

HDD_2_DECOMPRESS_URL    https://kojipkgs.fedoraproject.org/compose/rawhide/Fedora-Rawhide-20200320.n.0/compose/Spins/armhfp/images/Fedora-Minimal-armhfp-Rawhide-20200320.n.0-sda.raw.xz
HDD_2    Fedora-Minimal-armhfp-Rawhide-20200320.n.0-sda.raw
@AdamWill AdamWill force-pushed the AdamWill:fix-downloader branch from c9f7ebf to aab1e91 Mar 20, 2020
This regressed in #2736. When we want to download a compressed
file and extract it to produce the 'final' asset, we'll be doing
something like downloading 'http://example.com/file.img.gz' to
an asset called 'file.img'. After we download the asset, we write
it to a temporary file and feed that temporary file into
Archive::Extract, then extract it to the final asset location.
Archive::Extract uses the filename of the input file to determine
its type, so we should base the name of the temporary file on the
path component of the URL (where the compression is indicated) -
'file.img.gz' in this case - and not on the final target filename
(where the compression is not indicated) - 'file.img' in this
case. Before #2736, that's what the code did, but in #2736 it was
changed so the temporary filename is based on the target file.
I guess it was hard for @kraih to work this out without a real-
world example to understand how it's actually used in practice
(I think only Fedora uses this feature).

This returns the code to something similar to how it was before,
but using `path()->to_string` as @kraih's version did. It also
tweaks the test cases, which were clearly the wrong way round
once you know how this is supposed to work (it doesn't make
sense to expect extracting 'test.gz' to 'test.gz' to succeed,
but extracting 'test.gz' to 'test' to fail).

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill AdamWill force-pushed the AdamWill:fix-downloader branch from aab1e91 to 01f6178 Mar 20, 2020
@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Mar 20, 2020

coverage seems to be showing changes in an OpenID.pm this PR doesn't actually touch?

@okurz

This comment has been minimized.

Copy link
Member

okurz commented Mar 21, 2020

Yes, I think OpenID is not properly covered in tests.

@okurz
okurz approved these changes Mar 21, 2020
@kraih
kraih approved these changes Mar 21, 2020
Copy link
Member

kraih left a comment

Since you're most likely the only one using this feature, i guess we should trust that the tests now represent the correct behaviour. But please add more tests if something is still not covered. Otherwise we'll just end up breaking it again with the next optimisation or bug fix.

@okurz okurz merged commit 0d73b42 into os-autoinst:master Mar 21, 2020
14 checks passed
14 checks passed
OBS Package Build 8/8 processed
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: cache Your tests passed on CircleCI!
Details
ci/circleci: cache.fullstack Your tests passed on CircleCI!
Details
ci/circleci: checkstyle Your tests passed on CircleCI!
Details
ci/circleci: codecov Your tests passed on CircleCI!
Details
ci/circleci: developer Your tests passed on CircleCI!
Details
ci/circleci: fullstack Your tests passed on CircleCI!
Details
ci/circleci: scheduler Your tests passed on CircleCI!
Details
ci/circleci: t Your tests passed on CircleCI!
Details
ci/circleci: ui Your tests passed on CircleCI!
Details
codecov/patch No report found to compare against
Details
codecov/project 93.07% (target 85.7%)
Details
@AdamWill

This comment has been minimized.

Copy link
Contributor Author

AdamWill commented Mar 23, 2020

@kraih we only use it for one specific case, and this test should cover that fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.