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

Clean up temporary files when checksums don't match #412

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

benridley
Copy link
Contributor

@benridley benridley commented Aug 11, 2020

Noticed that the archive module was leaving behind quite a few files in /tmp when there were checksum mismatches.

This ensures that the temporary files will be cleaned up before throwing an error.

Pull Request (PR) description

This pull request removes the temporary files created by the archive module if the checksum fails to match. Previously, the module would throw an error and leave the temporary file in /tmp.

Noticed that the archive module was leaving behind quite a few files in /tmp when there were checksum mismatches.

This ensures that the temporary files will be cleaned up before throwing an error.
@benridley benridley changed the title Clean up temporary files checksums don't match Clean up temporary files when checksums don't match Aug 11, 2020
@ghoneycutt
Copy link
Member

@benridley Good idea! Could you please add a test so this can be merged. Acceptance tests are preferred. You can cause the situation where a tmp file would normally be left then check the existence of the file to show that it is not there.

@benridley
Copy link
Contributor Author

Hey @ghoneycutt, happy to add an acceptance test to get this merged.

I'm not really too familiar with Ruby testing. Is RSpec the right path to go down here or is that strictly unit testing? Cheers!

@vox-pupuli-tasks
Copy link

Dear @benridley, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@ghoneycutt
Copy link
Member

This module does not have acceptance tests setup and the errors are totally unrelated to this so merging.

@ghoneycutt ghoneycutt merged commit e1e9420 into voxpupuli:master Aug 26, 2020
@igalic
Copy link
Contributor

igalic commented Aug 26, 2020

This module does not have acceptance tests setup and the errors are totally unrelated to this so merging.

would this module benefit from acceptance tests?

@ghoneycutt
Copy link
Member

Ha, it sure would! Are you volunteering :)

cegeka-jenkins pushed a commit to cegeka/puppet-archive that referenced this pull request Mar 26, 2021
Clean up temporary files when checksums don't match
@@ -191,6 +191,7 @@ def transfer_download(archive_filepath)
archive = PuppetX::Bodeco::Archive.new(temppath)
actual_checksum = archive.checksum(resource[:checksum_type])
if actual_checksum != checksum
destroy(temppath)
Copy link
Member

Choose a reason for hiding this comment

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

destroy doesn’t take any arguments...
Fixed in #443

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

Successfully merging this pull request may close these issues.

4 participants