Skip to content

fix: Use tempfile in config-manager and downloader to ensure tmpfiles are always deleted#3178

Merged
Bravo555 merged 3 commits intothin-edge:mainfrom
Bravo555:improve/dont-leave-tmpfiles-when-downloading
Oct 9, 2024
Merged

fix: Use tempfile in config-manager and downloader to ensure tmpfiles are always deleted#3178
Bravo555 merged 3 commits intothin-edge:mainfrom
Bravo555:improve/dont-leave-tmpfiles-when-downloading

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented Oct 9, 2024

Proposed changes

Replace manually created tempfiles with .tmp suffixes that didn't properly get clean up on errors with tempfile which delete on drop.

Downloader and config manager should no longer leave temporary files if respective functions did not complete successfully.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
As setting ownership is a privileged operation and all tedge services
run as a regular user (except for tedge-write) the download crate
probably shouldn't try `chown`ing them, as it can result in an error.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/common/download/src/download.rs 82.85% 1 Missing and 5 partials ⚠️
crates/extensions/c8y_http_proxy/src/actor.rs 0.00% 1 Missing ⚠️
...rates/extensions/tedge_config_manager/src/actor.rs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

LGTM. I left a minor comment.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 9, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
508 0 2 508 100 1h29m49.892144999s

Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

@Bravo555 Bravo555 added this pull request to the merge queue Oct 9, 2024
Merged via the queue into thin-edge:main with commit 6e19875 Oct 9, 2024
@Bravo555 Bravo555 deleted the improve/dont-leave-tmpfiles-when-downloading branch October 10, 2024 09:06
@reubenmiller reubenmiller added the theme:configuration Theme: Configuration management label Dec 16, 2024
@reubenmiller reubenmiller changed the title Use tempfile in config-manager and downloader to ensure tmpfiles are always deleted fix: Use tempfile in config-manager and downloader to ensure tmpfiles are always deleted Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

theme:configuration Theme: Configuration management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants