-
Notifications
You must be signed in to change notification settings - Fork 272
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
ngclient updater: clean temp file if write fails #1757
ngclient updater: clean temp file if write fails #1757
Conversation
Pull Request Test Coverage Report for Build 1682855792
💛 - Coveralls |
Looks good to me. Probably adding one more assert that the file doesn't exist? |
3e34b0e
to
8008329
Compare
Yep, maybe you are correct that this way the test will be more complete. |
95d5d9f
to
3f4458f
Compare
Updated the pr with @jku suggestions. |
The CI failure is fixed in #1762. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks good. tempfile-not-found-during-exception case is untested but I'm fine with that.
When calling updater._persist_metadata() there is a possibility that writing the temporary file to storage can succeed, but moving it with os.replace could fail with OSError. Make sure we are removing the newly created temporary file in that case. Signed-off-by: Martin Vrachev <[email protected]>
3f4458f
to
6f91da6
Compare
I tried adding a new test patching |
Fixes #1575
Description of the changes being introduced by the pull request:
When calling updater._persist_metadata() there is a possibility that
writing the temporary file to storage can succeed, but moving it with
os.replace
could fail with OSError.Make sure we are removing the newly created temporary file in that case.
I tried testing the first case described in the issue, but I had troubles
mocking
write
as it was notos.write()
.Signed-off-by: Martin Vrachev [email protected]
Please verify and check that the pull request fulfills the following
requirements: