-
Notifications
You must be signed in to change notification settings - Fork 107
feat: port cleanup script to rust lang #819
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
a2d8423
24bb2a5
a6f8978
ef58d79
ff6e628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,2 @@ | ||
| PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin | ||
| 0 0 * * * root /etc/cron.cleanup_disk_space | ||
| 0 0 * * * root /usr/local/bin/clean-unused-checkouts |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could make sense to rename the parent folder from
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree 💯 . I like to break this task into 2 separate PRs, once this PR is approved and merged. I can raise the next PR to rename the directory. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah depends on what the infra team prefers - either renaming the folder to use it for multiple scripts, or making a separate folder for each script |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| use clap::Parser; | ||
| use std::fs; | ||
| use std::io; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::time::{Duration, SystemTime}; | ||
|
|
||
| /// Clean up unused build/target directories in user home directories | ||
|
amustaque97 marked this conversation as resolved.
Outdated
|
||
| #[derive(Parser, Debug)] | ||
| #[command(author, version, about, long_about = None)] | ||
| struct Cli { | ||
| /// Only print directories and their size, do not delete | ||
| #[arg(long)] | ||
| dry_run: bool, | ||
|
|
||
| /// The root directory to search for projects | ||
| #[arg(short, long = "root-directory", default_value = "/home")] | ||
| root_directory: PathBuf, | ||
|
|
||
| /// The maximum age of a project in days | ||
| /// | ||
| /// The CLI will only clean projects that have not been updated in the last `max-age` days. | ||
| #[arg(short, long = "max-age", default_value_t = 60)] | ||
| max_age: u32, | ||
| } | ||
|
|
||
| fn is_project_dir(dir: &Path) -> bool { | ||
| (dir.join("x.py").is_file() && dir.join("build").is_dir()) | ||
| || (dir.join("Cargo.toml").is_file() && dir.join("target").is_dir()) | ||
| } | ||
|
|
||
| fn find_cache_dirs(home: &Path) -> io::Result<Vec<PathBuf>> { | ||
| let mut result = Vec::new(); | ||
| for entry in fs::read_dir(home)? { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this implementation handle symlinks? This might be extreme, but I assume that users of dev-desktops could accidentally / intentionally create a symlink loop which might cause an infinite loop here. Since you already define walkdir as a dependency, perhaps it could be used here as well for the recursive search of project dirs (by default, it does not follow symlinks). The original issue has a snippet that could be used here I think (see
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, thanks for the idea. I have made the changes to prevent a symlink loop. |
||
| if path.is_dir() { | ||
| // Recursively search for project dirs | ||
| let mut stack = vec![path]; | ||
| while let Some(dir) = stack.pop() { | ||
| if is_project_dir(&dir) { | ||
| if dir.join("build").is_dir() { | ||
| result.push(dir.join("build")); | ||
| } | ||
| if dir.join("target").is_dir() { | ||
| result.push(dir.join("target")); | ||
| } | ||
| } else if let Ok(entries) = fs::read_dir(&dir) { | ||
| for e in entries.flatten() { | ||
| if e.path().is_dir() { | ||
| stack.push(e.path()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(result) | ||
| } | ||
|
|
||
| fn is_unused(dir: &Path, days: u64) -> io::Result<bool> { | ||
| let cutoff = SystemTime::now() - Duration::from_secs(days * 24 * 60 * 60); | ||
| let mut recent = false; | ||
| for entry in walkdir::WalkDir::new(dir.parent().unwrap_or(dir)) { | ||
| let entry = entry?; | ||
| if let Ok(meta) = entry.metadata() { | ||
| if let Ok(modified) = meta.modified() { | ||
| if modified > cutoff { | ||
| recent = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(!recent) | ||
| } | ||
|
|
||
| fn print_or_delete(dir: &Path, dry_run: bool) { | ||
| if dry_run { | ||
| let size = get_dir_size(dir); | ||
| match size { | ||
| Ok(bytes) => { | ||
| println!( | ||
| "{:.2} MiB\t{}", | ||
| bytes as f64 / 1024.0 / 1024.0, | ||
| dir.display() | ||
| ); | ||
| } | ||
| Err(_) => { | ||
| println!("{}", dir.display()); | ||
| } | ||
| } | ||
| } else { | ||
| println!("Deleting {}", dir.display()); | ||
| let _ = fs::remove_dir_all(dir); | ||
| } | ||
| } | ||
|
|
||
| fn get_dir_size(path: &Path) -> io::Result<u64> { | ||
| let mut size = 0u64; | ||
| for entry in walkdir::WalkDir::new(path) { | ||
| let entry = entry?; | ||
| if entry.file_type().is_file() { | ||
| size += entry.metadata()?.len(); | ||
| } | ||
| } | ||
| Ok(size) | ||
| } | ||
|
|
||
| fn main() -> io::Result<()> { | ||
| let cli = Cli::parse(); | ||
| let cache_dirs = find_cache_dirs(&cli.root_directory)?; | ||
| for dir in cache_dirs { | ||
| if is_unused(&dir, cli.max_age as u64)? { | ||
| print_or_delete(&dir, cli.dry_run); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth adding a specific test for find_cache_dirs and print_or_delete, as the former controls what will be scheduled for deletion, whereas the latter controls what actually gets deleted, to ensure no accidental changes end up scheduling wrong folders for deletion |
||
| use super::*; | ||
| use std::fs::{self, File}; | ||
| use std::io::Write; | ||
| use tempfile::tempdir; | ||
|
|
||
| #[test] | ||
| fn test_get_dir_size_empty() { | ||
| let dir = tempdir().unwrap(); | ||
| assert_eq!(get_dir_size(dir.path()).unwrap(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_get_dir_size_with_files() { | ||
| let dir = tempdir().unwrap(); | ||
| let file_path = dir.path().join("file1"); | ||
| let mut file = File::create(&file_path).unwrap(); | ||
| file.write_all(&[1u8; 1024]).unwrap(); | ||
| assert_eq!(get_dir_size(dir.path()).unwrap(), 1024); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_project_dir_xpy_build() { | ||
| let dir = tempdir().unwrap(); | ||
| File::create(dir.path().join("x.py")).unwrap(); | ||
| fs::create_dir(dir.path().join("build")).unwrap(); | ||
| assert!(is_project_dir(dir.path())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_project_dir_cargo_target() { | ||
| let dir = tempdir().unwrap(); | ||
| File::create(dir.path().join("Cargo.toml")).unwrap(); | ||
| fs::create_dir(dir.path().join("target")).unwrap(); | ||
| assert!(is_project_dir(dir.path())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_project_dir_false() { | ||
| let dir = tempdir().unwrap(); | ||
| assert!(!is_project_dir(dir.path())); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
(disclaimer: I don't know much of Ansible) is the intended deployment strategy to compile the binary locally, with the target set based on the dev-desktop architecture/os? Would it make sense to add the compilation step as an Ansible task/playbook? Otherwise a simple note in the README would work as well
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.
Me neither. Let's wait for the review from the team, and I will make the necessary changes.