Skip to content

Conversation

@theogayar
Copy link
Contributor

@theogayar theogayar commented Jan 21, 2026

Fixes #769

Updated

  • backup_printer_cfg > docstring + logic
  • backup_moonraker_conf > docstring + logic
  • backup_file > proper path for verbose output

target_path: removed the code that appended config_{timestamp}

target_name: removed the argument as it is was a duplicate of the subcall's logic. The backup_file method already contains logic to generate a timestamped filename ({stem}_{timestamp}{suffix}) if target_name is omitted.


Added a simple docstring to both methods, which could maybe be enhanced ?


Before:

Creating backup of /home/trident/printer_data/config/printer.cfg ...
[OK] Successfully backed up to '/home/trident/kiauh_backups/printer_data/config_20260121-021909'

Now:

Creating backup of /home/trident/printer_data/config/printer.cfg ...
[OK] Successfully backed up to '/home/trident/kiauh_backups/printer_data/printer_20260121-021909.cfg'

Edit: modified the docstring to use { } instead of < >, as the brackets where being rendered in the docstrings (as html headers..)

Edit 2: changed from destination_path to target_path to align with the rest of the project

@theogayar theogayar marked this pull request as draft January 21, 2026 15:27
@theogayar theogayar marked this pull request as ready for review January 21, 2026 15:37
@dw-0
Copy link
Owner

dw-0 commented Jan 24, 2026

Hi, thanks for the PR. Im not sure if i want to merge that change. The backup logic in KIAUH prior to a refactoring placed single files in the instances root directory with the date appended, exactly like your change would do it. I went away from it and rather placed the names with their original name in a timestamped subdir. That was intended and made consistent across the application (unless i missed some spots?!).

So a kiauh_backups/printer_data could look like this:
image
The config_<timestamp> directories could contain more than just a printer.cfg, depending on where the call for backup happens. If you create backups via the backup menu, a backup of the full config folder is created and in that case a subdir is actually required of course. If you for example uninstall Mainsail, a config_<timestamp> folder will be created because during the uninstallation process multiple things happen. The moonraker.conf gets patched and therefor will be copied to that directory, the printer.cfg will also be patched to remove the include statement of the mainsail.cfg and therefor is copied to the same directory.

I can understand your motivation, that a new subdir for a single file seems overkill, but im hesitant of changing the backup mechanics AGAIN, and to something that behaves incosistent across the application again. Do you have a better architectual idea for handling backups?

@theogayar
Copy link
Contributor Author

The config_<timestamp> directories could contain more than just a printer.cfg, depending on where the call for backup happens. If you create backups via the backup menu, a backup of the full config folder is created and in that case a subdir is actually required of course.

I appreciate the explanation, as I must admit I had no clue why the backup behavior followed this design. My goal was to reduce the 'clutter' of nested folders for simple, one-off backups, which is my use case, as I've actuallly never used the local backup from KIAUH and rather rely on a git workflow for that (this has me thinking of a new addition for a helper to set this up automatically lol...).

Do you have a better architectual idea for handling backups?

I hear your concern about consistency, and I think it is absolutely valid. However, I also believe that nesting a single file is not only overkill, but also that the output the backup services gives (unix-path like, without filename at the end) could be quite ambiguous, especially for users that aren't familiar with the cli.

What if we keep the current logic but add a 'flatten' check at the end of the backup call/session? If the resulting directory contains only a single file, the script could move that file up one level and remove the redundant folder. The only issue I can think of with this is that I'm not sure how to implement the backup 'session', as it would be quite difficult unless the backup service is modified to explicitely start and stop.

We could also potentially run something before exciting the script to check for every file that was backed up during the current run of KIAUH (we then need a logfile for backup transactions in order to avoid recursion throughout a potentially very large backup folder), and flatten if necessary. The only issue with this would be the potential delay introduced (mv is lightning fast anyways..)

In any case, I believe the edits to backup_file's output are still valid, as they do not interfere with the logic of the rest of the process, while making it more explicit where/what file was saved.

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.

Bug Report(backup_service): backup_moonraker_conf() and backup_printer_cfg create timestamped directory instead of timestamped filename

2 participants