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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove corrupt cache files and retry download 3 times #8806

Merged
merged 4 commits into from Apr 20, 2021

Conversation

blackliner
Copy link
Contributor

@blackliner blackliner commented Apr 15, 2021

Changelog: Feature: Validate checksum and retry download for corrupted downloaded cache files.
Docs: omit

For testing, it would be cool if this feature could be switched on&off (default on), so the old test could remain with the feature switched off. Or mocking the Server to actually deliver corrupt files could also be an option to improve test coverage of the functionality.

I am also unsure about the logging, I have seen quite a multitude of ways to inform the user, from using the python logging, to _out.warn up to custom actions in a queue 馃

Issue: #8807

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@CLAassistant
Copy link

CLAassistant commented Apr 15, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

The file_downloader.download() already implements a retry following user configuration, and if it finally doesn't succeed, it will raise anyway, so the for loop will not iterate again. So the proposed iteration was not exactly achieving a retry. I have realized that it is enough to check for corruption first, remove if corrupted and then call download. Such download will try to download and validate checksum up to X times (typically 3)

@blackliner
Copy link
Contributor Author

@memsharded thats amazing, thanks for the commit! Anything else I need to do to get this merged? Documentation?

@memsharded memsharded added this to the 1.36 milestone Apr 16, 2021
@memsharded
Copy link
Member

@memsharded thats amazing, thanks for the commit! Anything else I need to do to get this merged? Documentation?

Nop, it is fine, as this could be considered a good/better natural behavior of the download cache, I think it is not necessary to document it, I have read https://docs.conan.io/en/latest/configuration/download_cache.html, and seems we are good.

Assigned it to next 1.36.

@czoido czoido merged commit fab3b48 into conan-io:develop Apr 20, 2021
@blackliner blackliner deleted the remove-corrupt-cache-files branch April 20, 2021 13:55
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.

None yet

4 participants