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

Move merged PR job directories to trash_bin_dir #271

Merged
merged 61 commits into from
Aug 7, 2024

Conversation

Neves-P
Copy link
Member

@Neves-P Neves-P commented Jun 4, 2024

This PR adds a handler function for merged PRs that then moves the job directories into a trash_bin_dir, the location of which can be set via the config.

At the moment there is no scripting or automation to delete the contents of trash_bin_dir, this functionality will follow later.

The next step is testing the current implementation on a bot instance not running in software.eessi.io:

  • Merge events on PRs are detected correctly
  • Directories are moved to the appropriate trash_bin_root_dir and have the correct subdirectory structure that makes trash_bin_dir.

Most likely the config files need to be adjusted and things are likely to fail as the new additions have not run within the context of a bot instance. I will do so on Hábrók in Groningen and will report back on progress here.

trz42
trz42 previously requested changes Jun 5, 2024
Copy link
Contributor

@trz42 trz42 left a comment

Choose a reason for hiding this comment

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

Had a look at the PR. Looks good overall. Some small suggestions here and there. I wonder if we would want to keep the cleanup_pr.sh script. It might allow us to clean up manually. We could remove it later if the whole auto-cleanup works smoothly.

eessi_bot_event_handler.py Outdated Show resolved Hide resolved
eessi_bot_event_handler.py Outdated Show resolved Hide resolved
eessi_bot_event_handler.py Outdated Show resolved Hide resolved
eessi_bot_event_handler.py Outdated Show resolved Hide resolved
tasks/clean_up.py Outdated Show resolved Hide resolved
tasks/clean_up.py Show resolved Hide resolved
tasks/clean_up.py Outdated Show resolved Hide resolved
Neves-P and others added 19 commits June 6, 2024 10:35
Co-authored-by: Thomas Röblitz <[email protected]>
Co-authored-by: Thomas Röblitz <[email protected]>
true -> True
Better formatting for merge conflict errors
@Neves-P
Copy link
Member Author

Neves-P commented Jul 8, 2024

As it stands now the bot will:

  • Move the contents of the pr_XX directory with copytree(dirs_exist_ok = True) to trash_bin_dir. This includes the event_XXXX directory(ies) that contain the cloned PRs of the bot where the slurm.out files and other metadata also get dumped
  • Reports back which directories were moved in a comment containing the directory(ies) where the files were moved to (trash_bin_dir/YYYY.MM.DD)

I think this is ready for review. I have two questions to look into as part of the review:

  • Is there any case where dirs_exist_ok = True is problematic? I don't think so, as the PR numbers are unique and each PR will only be moved once, so setting this argument to True just makes it easier copying the entire directory tree without extra book-keeping, but maybe I'm wrong
  • Will merging PRs that span multiple months still be copied correctly? I think so, but I'm going to test that by mimicking such a case and merging a PR to see what happens. ✅ Confirmed in Update hpc.rug.nl-2023.01-eb-4.9.0-2022b.yml Neves-Bot/software-layer#47 (comment)

@Neves-P Neves-P marked this pull request as ready for review July 8, 2024 09:17
@Neves-P Neves-P changed the title [WIP] Move merged PR job directories to trash_bin_dir Move merged PR job directories to trash_bin_dir Jul 8, 2024
truib added 2 commits August 5, 2024 14:15
- putting configuration settings into a single section
- using shutil.move instead of shutil.copy (move should fallback to copy
  if moving is not doable)
@trz42
Copy link
Contributor

trz42 commented Aug 5, 2024

Looked at the PR and tested it. Added a few suggestions in Neves-P#2

While the suggested changes will break some symbolic links (we can look at fixing these later), they should make the procedure efficient by only moving data around (and only fallback to copying if moving wouldn't work).

@trz42 trz42 dismissed their stale review August 6, 2024 12:38

addressed

Copy link
Contributor

@trz42 trz42 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @Neves-P !

@trz42 trz42 merged commit 45755b7 into EESSI:develop Aug 7, 2024
7 checks passed
@Neves-P Neves-P deleted the feature/disk_cleanup branch August 7, 2024 10:14
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.

4 participants