fix: backup cli #345

n8marti · 2025-02-26T07:14:20Z

Get backup and restore working for CLI (prep for getting general backup/restore working)

Split backup/restore into own module and Include unittests
Ensure backup works to fixed and removable drives (snap pkg needs additional permission from user for removable drives)
Ensure restore works on clean Logos install
Ensure restore works on existing Logos install (i.e. overwriting existing data)
Re-publish CLI backup/restore options

n8marti · 2025-02-26T07:18:26Z

We might need/want to backup/restore Logos registry keys, as well as user data folders.

ctrlaltf24

Nothing new, some thoughts feel free to ignore, I know you're still working

ctrlaltf24 · 2025-02-26T07:19:14Z

ou_dedetai/app.py

+            return bar
+
        # De-dup
        if message != self._last_status:


Should the bar print even if the status didn't change?

We may consider changing the Downloading.... status to use a percentage instead of adding dots

ctrlaltf24 · 2025-02-26T07:19:43Z

ou_dedetai/control.py


 from . import system
-from . import utils
+# from . import utils


ctrlaltf24 · 2025-02-26T07:22:26Z

ou_dedetai/backup.py

+        while t.is_alive():
+            i += 1
+            i = i % 20
+            app.status(f"{message}{"." * i}\r")


nit: use progress in status rather than adding dots

ctrlaltf24 · 2025-02-26T07:23:00Z

ou_dedetai/backup.py

+    if mode == 'restore':
+        if not app.conf.logos_exe:
+            app.exit("Cannot restore, Logos is not installed")
+        dst_dir = Path(app.conf.logos_exe).parent


Should this use logos app data Dir instead?

I had the same question, but I think the Logos app should be installed before trying to restore a backup. Otherwise installing Logos over existing data directories might clobber them? Not sure. Also, is app.conf.logos_exe generic enough to be defined as the Verbum exe path as well?

Also, is app.conf.logos_exe generic enough to be defined as the Verbum exe path as well?

Yup

n8marti · 2025-02-26T15:43:14Z

Consider a file chooser for setting Backup dir:
The TUI one is a text field with mkdir. tui_app.py:L774

thw26 · 2025-03-01T13:35:43Z

Consider a file chooser for setting Backup dir: The TUI one is a text field with mkdir. tui_app.py:L774

cf. #228 for links to some potential curses file browsers.

ctrlaltf24 · 2025-03-06T20:10:55Z

Ran additional tests on 78aeef0:

Backup before user login -> success
Backup before user login, then login, then restore backup before login -> not logged in
Backup with logos account one, restore backup before login, login with second logos account, restore logos account one backup -> logos account one restored
Backup one one directory, change install dir, install into that directory, restore -> success

Made a PR into this branch with a refactor to using python inheritance rather than setting source/dest dir manually. #360

ctrlaltf24

Approved, although would like to see #360 merged into this first.

ctrlaltf24 · 2025-02-27T03:56:18Z

ou_dedetai/config.py

        question = "New or existing folder to store backups in: "
        options = [PROMPT_OPTION_DIRECTORY]
-        output = Path(self._ask_if_not_found("backup_dir", question, options))
-        output.mkdir(parents=True, exist_ok=True)


What you have here is fine, or you could move https://github.com/FaithLife-Community/OuDedetai/pull/345/files#diff-07bb26aae0eae6d2749c28c992609c0619b8a27c628a8dc5efc075308f20181fR29-R35 here

Rational for the logic here was the return type is Path, so it'd be nice if it exists. Nice to have valid values returned from Config

ctrlaltf24 · 2025-02-27T03:58:31Z

ou_dedetai/utils.py



-def get_path_size(file_path):
+def get_path_size(file_path: Path|str) -> int:


nit: no need to change, but os.walk https://www.geeksforgeeks.org/os-walk-python/ is well designed for this task. Just FYI, not requiring you to change

sure. Is it significantly faster? Less memory and/or CPU intensive? Or is there some other benefit to os.walk over Path.rglob?

doesn't require recursion is the main benefit which may be inconsequentially faster

rather than initializing destination_dir to none, then setting it, use abstract functions to populate them.

thw26 · 2025-03-07T23:53:47Z

ou_dedetai/tui_app.py

+            backup.backup(self)
        else:
-            control.restore(self)
+            backup.restore(self)


We need to re-enable backup and restore in the menu below, lines 983–984.

Perhaps I misunderstood, but this PR is not intended to fix #197, only partially so, correct?

The way I understood it was an improvement, related to #197, but not aimed at fixing it. I looked to the PR description to determine scope rather than the issue

Tested: - clean install - clean install, backup #1, login, logos open, backup #2, restore #1, logos at login, close, restore #2, logos open

Discussed in other channel - they're willing to merge this

n8marti added 3 commits February 26, 2025 08:03

chore: move backup/restore to own module

0e2e054

feat: initial work for CLI progress bar

be36cfb

fix: log error if no write perms on backup_dir

cde9156

ctrlaltf24 reviewed Feb 26, 2025

View reviewed changes

n8marti added 6 commits February 26, 2025 13:13

refactor: use backup/restore classes

84255d1

fix: do dir creation in backup/restore action for better error handling

d9b50f2

feat: initial test_backup.py

8882bf2

chore: add typing hints; do some cleanup

4eba187

fix: default to most recent backup but off list of all backups

eba8c93

chore: un-hide backup/restore CLI options

68d2152

chore: add some unittests

66ab85b

n8marti marked this pull request as ready for review February 26, 2025 16:19

n8marti requested review from ctrlaltf24 and thw26 February 26, 2025 16:19

fix: bad variable name

f8365ab

Merge branch 'main' into fix-backup-cli

78aeef0

ctrlaltf24 changed the title ~~Fix backup cli~~ fix: backup cli Mar 6, 2025

ctrlaltf24 approved these changes Mar 6, 2025

View reviewed changes

refactor: use abstract base class in backup

f3bb6e2

rather than initializing destination_dir to none, then setting it, use abstract functions to populate them.

thw26 previously requested changes Mar 7, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-backup-cli

716d850

Tested: - clean install - clean install, backup #1, login, logos open, backup #2, restore #1, logos at login, close, restore #2, logos open

ctrlaltf24 merged commit 53f90db into main Mar 10, 2025
1 check passed

ctrlaltf24 deleted the fix-backup-cli branch March 10, 2025 15:36



		def get_path_size(file_path):
		def get_path_size(file_path: Path\|str) -> int:

fix: backup cli #345

fix: backup cli #345

Uh oh!

Conversation

n8marti commented Feb 26, 2025 • edited by ctrlaltf24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n8marti commented Feb 26, 2025

Uh oh!

ctrlaltf24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8marti commented Feb 26, 2025

Uh oh!

thw26 commented Mar 1, 2025

Uh oh!

ctrlaltf24 commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctrlaltf24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

n8marti commented Feb 26, 2025 •

edited by ctrlaltf24

Loading

ctrlaltf24 commented Mar 6, 2025 •

edited

Loading