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

Migrate to tempfile for libcontainer and youki crate #1887

Merged
merged 3 commits into from
May 9, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented May 8, 2023

As explained in #1883, we would like to move away from rolling out our own tempdir implementation. This is the first PR to migrate libcontainer and youki crate. The libcgroup and integrate_test will follow.

A few notes:

First, I remove using test name to create unique tempdir. Since the tempdir are designed to go away once out of the scope, using random names is enough to create unique tempdir. The directory will be removed regardless of test successful or not, so there is no reason to use human readable names.

Second, a number of test is partly re-written so it works better with the tempdir crate. Notably, we introduced scopeguard and defer!() to recreate the Drop delete a specific path that is not inside a tempdir logic. I find this to communicate the intent of the test more clear.

Third, for Rootless, I decided to introduce dependency injection to make the uid/gid mapping path configurable, so test can pass in a path instead of hardcode to a specific path.

@yihuaf yihuaf requested review from utam0k and a team May 8, 2023 03:06
@yihuaf yihuaf self-assigned this May 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2023

Codecov Report

Merging #1887 (a83a5fd) into main (5c31fae) will increase coverage by 0.03%.
The diff coverage is 91.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1887      +/-   ##
==========================================
+ Coverage   67.57%   67.61%   +0.03%     
==========================================
  Files         125      125              
  Lines       14199    14204       +5     
==========================================
+ Hits         9595     9604       +9     
+ Misses       4604     4600       -4     

@yihuaf yihuaf force-pushed the yihuaf/tempfile branch from 2d4999e to a83a5fd Compare May 8, 2023 05:04
crates/youki/src/main.rs Outdated Show resolved Hide resolved
crates/youki/src/main.rs Outdated Show resolved Hide resolved
Signed-off-by: yihuaf <[email protected]>
@yihuaf yihuaf requested a review from utam0k May 8, 2023 19:13
@yihuaf
Copy link
Collaborator Author

yihuaf commented May 8, 2023

@utam0k Addressed the review. PTAL :)

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks

use std::fs;
use std::path::{Path, PathBuf};

pub fn determine(root_path: Option<PathBuf>) -> Result<PathBuf> {
Copy link
Member

Choose a reason for hiding this comment

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

😍

@utam0k utam0k merged commit 654c7f4 into youki-dev:main May 9, 2023
@yihuaf yihuaf deleted the yihuaf/tempfile branch May 9, 2023 14:45
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.

3 participants