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

Include the OS release info in backup directory names #780

Open
wants to merge 30 commits into
base: m_778
Choose a base branch
from

Conversation

rpi-simonz
Copy link

Adding to issue #778 this is my attempt implementing a different naming scheme of the backup directories which includes the OS version.

Using

raspberrypi@debian12-rsync-backup-20240416-094106

instead of

raspberrypi-rsync-backup-20240416-094106

This way every time a new OS version is saved a new backup directory is created/used and thus a new full backup is created.

Possibly already existing backups with the old name (without OS release info) are simply ignored during smartRecycle and while checking the number of backups to keep.

I successfully tested backup and restore in different configurations.

@framps framps added this to the 0.7.0 milestone Sep 25, 2024
@framps framps self-assigned this Sep 25, 2024
@rpi-simonz rpi-simonz changed the base branch from master to m_778 September 25, 2024 18:04
@rpi-simonz
Copy link
Author

Regarding this ...

Possibly already existing backups with the old name (without OS release info)
are simply ignored during smartRecycle and while checking the number of backups to keep.

... I'll check the other possible option: including the old ones.

Probably tomorrow. Stay tuned.

@framps framps changed the title Include the OS release info in backup names Include the OS release info in backup directory names Sep 25, 2024
@framps
Copy link
Owner

framps commented Sep 29, 2024

Appreciate this PR. It's very useful to include the OS release in the backup path name.

Unfortunately I'm busy testing the next release 0.7.0 and therefore don't accept the PR as of now. Please be patient. I'll accept the PR when I finished my tests for next release and then add this nice PR

@rpi-simonz
Copy link
Author

I'm still working on some implementation details of this PR.
So it's a perfect fit that you are busy elsewhere for some time! ;-)

@rpi-simonz
Copy link
Author

rpi-simonz commented Oct 1, 2024

It's done. As far as I can tell... (Edit-1: ROFL! It's never done!)

The old backups without OS release info in its name are re-included into recycling strategy again.

But considering the current discussion in https://forum-raspberrypi.de regarding partitionbased backups and their handling during recycling it might be an idea to

  • add an indicator for those backups into the directory name as well; like rsync-pb-backup (or better without an additional dash: rsyncpb-backup) and treat them differently

Of course that's another story.

Edit-2: In the meantime the strategy has been changed. Old-type backups are old backups. Let them alone!

@rpi-simonz
Copy link
Author

So, since in the meantime the strategy had been changed (again) it's now like this:

Old-type backups (without OS-release info in its name) are old backups.
They are not touched/deleted/recycled automatically.

But there is a - hopefully - clear message for the user about them: They can be deleted manually.

I used and created new MSG_ messages for writeToConsole as good as I could.
Probably the numbering has to be adapted - I didn't know your preferences there.

@framps
Copy link
Owner

framps commented Oct 5, 2024

But there is a - hopefully - clear message for the user about them: They can be deleted manually.

I frankly thought to document this but usually docs are not read - so it's a very good idea to write a message to inform about this change.

Probably the numbering has to be adapted - I didn't know your preferences there.

No worries - I also have added some new messages. Message numbers can be updated very easy 😉

@framps framps assigned rpi-simonz and unassigned framps Oct 5, 2024
@framps framps added the feature label Oct 5, 2024
@rpi-simonz
Copy link
Author

I enhanced the message about the directory name change by referring to a raspiBackup version number.
Of course that number needs to be checked again on implementation.

@rpi-simonz
Copy link
Author

Commit 778ffc6 (Fix some indentation inconsistencies) above isn't exactly related to this topic here. But it slipped in...

@framps framps self-requested a review October 7, 2024 17:25
raspiBackup.sh Outdated Show resolved Hide resolved
raspiBackup.sh Show resolved Hide resolved
raspiBackup.sh Show resolved Hide resolved
@framps
Copy link
Owner

framps commented Oct 7, 2024

Please update raspiBackup7412Test.sh in the test directory to use the new backup directory name.

Don't fiddle with the tests ❗ . It created a lot of headache to create them and I don't want you to get the same headache 😉 .

Just make sure they succeed with the new backup directory name. Would be great if you can make the test to run also on another more powerful Linux system. I executed this test on my desktop instead of a Raspberry.

@rpi-simonz
Copy link
Author

At least raspiBackup7412Test.sh is running now - without complaints.
But I'm not sure whether I have done it completely right. The directory handling isn't easy to understand there. See the TODOs.

For now I won't continue with that test script. Other parts seem to be more important.

@framps
Copy link
Owner

framps commented Oct 8, 2024

That's what I expected: You had to update the directory name only and not to change anything in the test logic. I agree it's complicated code. But the test now finishes again successfully and that's important because you updated the SR code and we have to make sure the SR code still works correctly 😉

test/raspiBackup7412Test.sh Outdated Show resolved Hide resolved
test/raspiBackup7412Test.sh Outdated Show resolved Hide resolved
test/raspiBackup7412Test.sh Outdated Show resolved Hide resolved
Get getOSRelease by sourcing raspiBackup.sh.
Add test to check for NOT deleting old-named backups.
And some smaller changes.
echo "$DEFAULT_REPORT_COUNTER" > "$report_counter_file"
return
fi
rf=( $(<$report_counter_file) )
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason you use rf=( $(<$report_counter_file) ) instead of just rf= $(<$report_counter_file) to read the file contents? I see no reason to use an array here.

if (( $rfn > 0 )) ; then
writeToConsole $MSG_LEVEL_MINIMAL $MSG_OLD_NAME_BACKUPS_COUNTER_INFO "${rfn}"
else
writeToConsole $MSG_LEVEL_MINIMAL $MSG_OLD_NAME_BACKUPS_COUNTER_INFO_BYE
Copy link
Owner

Choose a reason for hiding this comment

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

It's kindly to say good bye 😄 . But I would just stop the report without any message. That's also done by the restore test reminder and then raspiBackup reminders work consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants