Modified the deletion to trim backups for each specific backup target.

[shortchanged13]

This is my first ever commit/pull request. Please let me know what I can do better.

I see that you have already moved the backup type determination to inside the backup loop. I also made the change in this request. I didn't know how to base my changes off of your changes.

I have also put a loop around the backup deletion process. It will now look at the backups that have been done per backup target. Those two deletion sections could have been pushed into the backup target loop but I choose to wrap them separately since only one --list-archive would be needed.

Thank you for the script. Its quick, simple and gets the job done. Let me know if there is anything else you would like me to take a look at.

Scott

[alexjurkiewicz]

Thanks, and congrats on your first PR! I'll take a closer look tonight, but this feature is very exciting and will fix up (imho) the biggest wart of acts.

[alexjurkiewicz]

Sorry for the delay. I think this is great! There are a few comments I have.

[alexjurkiewicz]

extra newline

[alexjurkiewicz]

The pattern (and all others that now have .+ in the middle should end with \$, just to be safe.

[shortchanged13]

Thanks. Can you explain the difference between your last two comments? Good catch on ending the grep, not sure why I missed that. I've made the changes and added the following for your last two changes: @@ -179,13 +179,13 @@ if [ "$backuprc" != "0" ]; then die "acts-tarsnap-error One of the backups failed -- not deleting old backups" fi - for dir in $backuptargets; do + log_debug "message=\"Checking $dir for old backups to delete\"" nicedirname=$(echo "$dir" | tr -d '/') # We don't delete any yearly backups Do I just push the changes or do I do another PR? After this change is complete I was thinking about making the number of backups to keep configurable. Anything else you would like? Scott

…

On 2019-02-06 23:57, Alex Jurkiewicz wrote: @alexjurkiewicz requested changes on this pull request. Sorry for the delay. I think this is great! There are a few comments I have. ------------------------- In acts [1]: > @@ -180,37 +179,41 @@ if [ "$backuprc" != "0" ]; then die "acts-tarsnap-error One of the backups failed -- not deleting old backups" fi -# We don't delete any yearly backups - -# We keep 12 monthly backups -monthlybackups=$(echo "$archives" | grep "$hostname-monthly-" | cut -d_ -f1 | uniq | sort -rn) -if [ "$(echo "$monthlybackups" | wc -l)" -gt 12 ]; then - log_debug 'message="More than 12 monthly backups, deleting the oldest"' - echo "$monthlybackups" | tail -n +13 | while read -r archiveprefixtodel; do - log_verbose "message=\"Deleting backup prefix $archiveprefixtodel*\"" - echo "$archives" | grep -E "^$archiveprefixtodel" | while read -r archivetodel; do - log_debug "message=\"Deleting backup $archivetodel\"" - $tarsnap -d -f "$archivetodel" + extra newline ------------------------- In acts [2]: > @@ -161,6 +146,20 @@ backuprc=0 # Notice any failed backups for dir in $backuptargets; do archive_starttime=$(date +%s) nicedirname=$(echo "$dir" | tr -d '/') + # Determine the archive type to create + if echo "$archives" | grep -E -q "^$hostname-yearly-$year-.+-$nicedirname"; then The pattern (and all others that now have .+ in the middle should end with \$, just to be safe. ------------------------- In acts [3]: > @@ -180,37 +179,41 @@ if [ "$backuprc" != "0" ]; then die "acts-tarsnap-error One of the backups failed -- not deleting old backups" fi -# We don't delete any yearly backups - -# We keep 12 monthly backups -monthlybackups=$(echo "$archives" | grep "$hostname-monthly-" | cut -d_ -f1 | uniq | sort -rn) -if [ "$(echo "$monthlybackups" | wc -l)" -gt 12 ]; then - log_debug 'message="More than 12 monthly backups, deleting the oldest"' - echo "$monthlybackups" | tail -n +13 | while read -r archiveprefixtodel; do - log_verbose "message=\"Deleting backup prefix $archiveprefixtodel*\"" - echo "$archives" | grep -E "^$archiveprefixtodel" | while read -r archivetodel; do - log_debug "message=\"Deleting backup $archivetodel\"" - $tarsnap -d -f "$archivetodel" + +for dir in $backuptargets; do Can you add a log_debug message specifying which backup target is being checked at the start of the loop? ------------------------- In acts [4]: > @@ -161,6 +146,20 @@ backuprc=0 # Notice any failed backups for dir in $backuptargets; do Can you add a log_debug message specifying which backup target is being processed at the start of the loop? -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [5], or mute the thread [6]. Links: ------ [1] #28 (comment) [2] #28 (comment) [3] #28 (comment) [4] #28 (comment) [5] #28 (review) [6] https://github.com/notifications/unsubscribe-auth/AtJRvTqjkjQozjyZpD8ACU3RcJQobM70ks5vK7IegaJpZM4aihl-

[alexjurkiewicz]

You can push more commits to the branch you've opened this PR from, and they will automatically show up in the PR!

For future reference, please use the github UI to respond to PR reviews, you can thread the conversations that way. But it's no big deal here because the review is quite small.

The only difference between the last two comments is what the log message would say. I was imagining something like:
log_debug "message=\"Backing up $dir\""
and (like you suggested):
log_debug "message=\"Checking $dir for old backups to delete\""

[shortchanged13]

Requested changes have been made and pushed. Thanks for clarification on the debug messages. I didn't realize it was two different loops.

[alexjurkiewicz]

Looks good!

[alexjurkiewicz]

I think this is ready to merge! I don't currently use acts so I can't test it myself -- have you been running this for a few days? Does adding new backup paths work properly?

[alexjurkiewicz]

One other thought: can you please update the README to mention that if you remove a backup target, you'll have to delete its old backups by hand?

[alexjurkiewicz]

Thanks for the contribution! It's merged, and acts is now v1.4b1

[shortchanged13]

Thank you. I missed your messages. I have had it running daily.