-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
ruby provider: ensure cleanup happens #474
Conversation
7b3e6f5
to
1de7888
Compare
1de7888
to
207e6a6
Compare
Hi @pillarsdotnet, thanks for the PR. can you please rebase against our latest master branch? |
207e6a6
to
3331ced
Compare
Rebased. |
@bastelfreak -- Would you please review? |
3331ced
to
752a23b
Compare
@kenyon -- Thanks for fixes. |
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.
LGTM, would be nice to have a test for this though.
752a23b
to
7571f6f
Compare
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.
LGTM! I pushed a commit to make rubocop happy.
raise(Puppet::Error, "Download file checksum mismatch (expected: #{checksum} actual: #{actual_checksum})") | ||
end | ||
end | ||
|
||
move_file_in_place(temppath, archive_filepath) | ||
ensure | ||
FileUtils.rm_f(temppath) if File.exist?(temppath) |
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.
Technically you can rm_f a file that does not exist (because force is only about ignoring errors). Checking for existence does not hurt though, so I am fine with this.
Pull Request (PR) description
Ensures that tempfile used for downloading gets deleted.
Ensures that the
cleanup
param is honored.This Pull Request (PR) fixes the following issues
Fixes #328