feat: port cleanup script to rust lang #819
Conversation
|
|
||
| - name: Install clean-unused-checkouts binary | ||
| copy: | ||
| src: "{{ playbook_dir }}/../../../../setup-deploy-keys/target/release/clean-unused-checkouts" |
There was a problem hiding this comment.
(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.
Me neither. Let's wait for the review from the team, and I will make the necessary changes.
There was a problem hiding this comment.
It could make sense to rename the parent folder from setup-deploy-keys to something more generic, such as cli-utilities or similar, if the idea is to have multiple simple CLI tools inside this folder
There was a problem hiding this comment.
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.
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
| 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.
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 find_artifact_directories definition)
There was a problem hiding this comment.
Oh, thanks for the idea. I have made the changes to prevent a symlink loop.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| result.sort(); | ||
| result.dedup(); |
There was a problem hiding this comment.
Nitpick: since we don't want duplicates and we don't care about ordering, we could use a hashset instead of a vec
2 participants
This pull request replaces the legacy Bash script for cleaning unused build and target directories on
dev-desktopsystems with a new Rust implementation (clean-unused-checkouts). The Rust binary is now built in thesetup-deploy-keyscrate and distributed to target systems via Ansible. The Bash script has been removed from the repository.