-
Notifications
You must be signed in to change notification settings - Fork 7
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
omv-backup-vm script only deletes old backup files but doesn't remove them from /etc/omv-backup-vm.list #23
Comments
Found the problem: openmediavault-kvm/usr/sbin/omv-backup-vm Line 272 in c17608f
This adds a "/" to the openmediavault-kvm/usr/sbin/omv-backup-vm Line 277 in c17608f
|
This PR (#24) should close this issue. |
I'm confused by this fix. dirname will always return the parent directory without a trailing slash of the keepDir even if keepDir ends in slashes or not. Then a trailing slash is added to a value that couldn't have an ending slash. How would the log file have an entry without a trailing slash that sed fails to find? |
You are right. The problem starts earlier though. With the variable
I have been running the script with openmediavault-kvm/usr/sbin/omv-backup-vm Line 254 in c17608f
And because My PR adds a trailing slash to Correct me if I am wrong or if I misunderstood anything. After changing the script in my installation it has been working as intended though. |
I didn't look at the whole PR. It was a lot of changes for something simple and should be a separate PR for the verifying part (I would rather that be in a separate script). Why not just improve the sed to something like this? sed -i "|${delDir%/}/?,${backupVm},${delDate}|d" "${listFile}" |
Yes. That's probably better than my
No problem. The change for the slash was just one line. The biggest part is the script that looks for deprecated entries and deletes them. I didn't dare to propose a separate script. Of course it's not necessary to run that part every time. Doing it once after including the new sed pattern is probably enough to get rid of deprecated entries. I'm not experienced enough to implement that in a good way though. |
Ok, I will push the sed change and work on a separate script that verifies that the backups for the logged entries actually exists and removes them if they don't exist. |
Thanks a lot for all your work and your help! Just one addition, in case it's not intentional: Would it not be nicer to also adjust the grep command that preceeds the now changed sed command? The result is the same but it would be more uniform. |
Oops, I forgot that. Thanks for pointing it out. |
When called directly with the "-k" flag the
/usr/sbin/omv-backup-vm
script adds the new backup correctly to/etc/omv-backup-vm.list
and correctly removes old backup files and folders as set with "-k" but it doesn't remove the deleted backups from the list causing them to remain visible in the web GUI.The text was updated successfully, but these errors were encountered: