bit-team
feat(cli): Status information about all backup profiles
Implemented the 'status' command to provide a snapshot of the status of all profiles.
options: -h, --help show this help message and exit --profile NAME a more detailed summary of the profile with the name, NAME --profile-id ID a more detailed summary of the profile with the id, ID --issues show only profiles with errors on most recent run or no snapshot history at all --human-readable, -r print in human readable format
Consider adding a CHANGELOG entry. Run "codespell" to check for typos.
Close #1018
I've commited some changes (and converted pr to draft):
- fixed --issues switch
- changed default to human readable output
- added --json switch for machine readable output
- implemented checking status on ssh drives
I've been trying different layouts for the output and I think it looks neat now but I was wondering if I should add a --compact switch or something similar in case the user has lots of profiles, what do you think?
Is there any other information you want to be displayed in the profile specific view (or the overview)?
Looks good.
There is a bug in BIT that helps you somehow. :rofl: Your connections cause INFO logging outputs on terminal. Usually this polute the json string. But BIT has the bug that every logging output, not matter of its level, is on stderr.
So your json output and the log output is separated.
...
STDERR: INFO: Connecting to: unittest
STDERR: INFO: mount ssh_encfs: user@localhost:/home/user/Downloads/bit_test_ssh_encfs on /home/user/.local/share/backintime/mnt/8D19350B/mountpoint
STDERR: INFO: unmount ssh_encfs: user@localhost:/home/user/Downloads/bit_test_ssh_encfs from /home/user/.local/share/backintime/mnt/8D19350B/mountpoint
STDOUT: {
STDOUT: "Main profile": {
STDOUT: "Last snapshot": "2023-08-13 15:30:34",
STDOUT: "Successful": true
STDOUT: },
...
We can keep it that way. Re-write the logging machine, using Pythons in-build logging, is also on my list. But it is that big that I won't touch it yet.
Usually I would say that your feature should somehow silence the log output. But with this BIT bug there is no need. Let's keep it simple.
Do you have any suggestions on other data to be included or other changes to be made? Otherwise, should I start to look at incorporating this funcitonality into the gui?
On a side note, thank you for including me in the list of contributors for the release candidate. Just a heads-up for possible future mentions, I recently changed my github username from mooresamuel to s4moore.
Hello Samuel, Please excuse my late reply. I was offline because of technical reasons.
Do you have any suggestions on other data to be included or other changes to be made?
No, not yet. I think it is OK the way it is. It is a good beginning and can be extended later.
Otherwise, should I start to look at incorporating this funcitonality into the gui?
Having this in the GUI wasn't my use case. But I am open to it if you want to have this in the GUI.
Before coding something I would recommend that you create a mockup we can discuss. Create a new issue for this please.
I do use draw.io or Qt Creator for mock ups. But you can also draw by hand and take a photo. Every kind of visual would help.
Hi Christian,
No problem, I don't feel like you were too long in replying.
I was looking at the original issue (#1018) again and saw that you wanted to be able to check the status of a drive if it was not online so I have updated the approach. It now creates a log file and updates that each time it finishes a backup so the info is available when a drive is offline. Happy to go back to the previous version if you would prefer that, just let me know.
Thanks
~~I'll work on a modification of the statefile code to better support your PR.~~
~~Currently BIT has one statefile related to backintime-qt. A statefile for backintime-common is missing. In my opinion your backup-jobs-status information should be written into such a state file.~~
Sorry, I still mangling this PR in my head. Modifyfing the state-file code wasn't such a good idea as I thought before.
Can you explain me how do you distinguish if a backup job was successful or not?
Hi Christian,,
Sorry for not replying sooner, I have been quite busy the past few weeks, congratulations on another release!
Yes, after each backup, the log is parsed and the status file is updated, if it exists. If there is no status file, all profiles are checked and the status for all profiles is recorded.
A backup is counted as having been successful if there were files written without any errors. If a backup is run and no files are written (or files are written but with errors), this will be recorded as the "last run" so the user will be able to see the last time a backup job was run as well as the "last successful" backup (changes were made without any errors). OBviously if these results aren't as expected, the user could take a deeper dive through the logs as per normal.
Looking back at the code now, I realise that it is not very clear, perhaps I should refactor it? I have also just discovered an error in the logic so it does need a bit of work anyway.
I also wanted to add some tests but I struggled to get my head around the generic test classes and how to implement them and was worried that I might write tests that would pass on my machine but not pass on others due to my misunderstanding something. Should I attempt to write tests anyway? (And if so, do you have any advice or guidance on the approach I should take?)
Yes, after each backup, the log is parsed
Mhm... But why? BIT itself somehow determines if a backup was successful or not. But I am not sure how it works. Can't you use the same mechanic? I think parsing a log file is bit to much overhead for such a simple task.
I also wanted to add some tests
Let's skip this in the first place. I'll think about it when the code is finished.
Yes, after each backup, the log is parsed
Mhm... But why? BIT itself somehow determines if a backup was successful or not. But I am not sure how it works. Can't you use the same mechanic? I think parsing a log file is bit to much overhead for such a simple task.
As I understand, BIT considers a backup to be successful as long as there are no errors, even if no files are written. I thought it would be useful to know the last time that files were backed up without any errors but yes, this does add some overhead and maybe it's not really necessary.
Yes, I can use the same mechanic, I will work on another solution.
I might misunderstand something?
As I understand, BIT considers a backup to be successful as long as there are no errors, even if no files are written.
But if no files are written, than there is no new backup.
I thought it would be useful to know the last time that files were backed up without any errors but yes
Isn't this the same as the previous case?
Hi Christian,
Sorry, it's been a while since I worked on this. Yes, you are right. I was thinking of the log file that is written (in ~/.local/chare/backintime ) after any backup attempt (which wouldn't be an issue if I was only using the builtin mechanism).
Looking back at my code, I have actually used the builtin mechanism but also check the logs to see how many files were changed and record the error detail. I guess I was just trying, maybe too hard, to include any information which might be useful. Shall I remove that from the code then?
Edit: I also thought it would be useful for the user to be able to see the timestamp for the most recent back attempt (even if no changes were made) to confirm jobs were being run as scheduled but I guess there are other ways to do this as well
Sorry, it's been a while since I worked on this.
No problem. It is the same on my site because I try to focus on some other things.
Looking back at my code, I have actually used the builtin mechanism but also check the logs to see how many files were changed and record the error detail. I guess I was just trying, maybe too hard, to include any information which might be useful. Shall I remove that from the code then?
It is not bad trying it "hard". ;) Its easier to find boundaries this way. No matter that the feature might be useful it is a bit to much for the first implementation. I would say you can remove it from this PR. But don't delete it. Save it somewhere else. Maybe we can come back to it later. I like to keep it simple in the first place to get an idea about how things might work and how the users react to it. Also it is always about maintainability.
Edit: I also thought it would be useful for the user to be able to see the timestamp for the most recent back attempt (even if no changes were made)
That is a very nice feature! The timestamp of the last job run, even if no changes are made, should be kept. This info should stay. Distinguish between the timestamp of the last backup made and the last run (even without changes) is very useful I think.
How BIT handle snapshots logs is also new to me. You might still know it but I discovered something new to me. So this is just for your (and my) information.
In this example you see in the timeline (1.) the last backup from 11:49:45. But clicking on Last Log View (2.) you get a log file with a timestamp from 11:50:00 (3.). This is because I run a backup job, without any changes ("Done, no backup needed.").
I have implemented the requested changes and tried to make the code more clear. All seems to be working fine from my side but please let me know if there are any other changes I should make 😄
Thank you. I'll take a look. It might be that I push some commits to this PR. So please wait.
I'm a bit worried that I'm annoying you with all my comments and suggestions.
Haha not at all, I'm a bit worried that I'm annoying you with writing code that isn't hitting the mark! 🙃
- There where some linter warnings, I fixed. Please install
ruff(maybeflake8) andpylinton your local machine to see them while running tests.
This is strange, I have run checks with all of them and didn't get any errors...
test@test-VirtualBox:~/backintime/common$ pytest test/test_lint.py
================================================= test session starts ==================================================
platform linux -- Python 3.12.3, pytest-7.4.4, pluggy-1.4.0
rootdir: /home/test/backintime/common
plugins: pyfakefs-5.7.4
collected 6 items
test/test_lint.py ...... [100%]
============================================= 6 passed in 72.69s (0:01:12) =============================================
test@test-VirtualBox:~/backintime/common$ pylint status.py
--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)
test@test-VirtualBox:~/backintime/common$ ruff check status.py
All checks passed!
test@test-VirtualBox:~/backintime/common$ flake8 status.py
test@test-VirtualBox:~/backintime/common$
Do you have any idea what I'm doing wrong so I can try to avoid this in the future?
- The test
test/test_takeSnapshot.py::TakeSSH::test_errorfails. Also some other tests, but maybe for the same reason. And I don't understand why but I think the decorator is somehow involved.
Okay, I will remove the decorator. I was only trying to avoid having the same call multiple times but I see from your comments that you're okay with that for now so it's an easy fix 😄
I'm a bit worried that I'm annoying you with writing code that isn't hitting the mark! 🙃
That is the usual case. Even with myself. Just look at the PRs I am opening. There are often a lot of iterations until I merge them.
This is strange, I have run checks with all of them and didn't get any errors... test/test_lint.py ...... [100%]
Try with "-v" switch. The linter tests are skipped by default (only on local machines but not TravisCI) if the linters are not available in the system. The tests don't fail. My intention was not to annoy fresh contributors with pedantic linter warnings. 😄
But now all linter warnings are fixed by me. This might be the reaons why the tests passing?
- The test
test/test_takeSnapshot.py::TakeSSH::test_errorfails. Also some other tests, but maybe for the same reason. And I don't understand why but I think the decorator is somehow involved.Okay, I will remove the decorator. I was only trying to avoid having the same call multiple times but I see from your comments that you're okay with that for now so it's an easy fix 😄
But I think removing the decorator won't make the tests pass. There seems to be more behind.
btw: Not sure if you read the mailing list. Because you are currently involved with the CLI interface, you might have an opinion about this question?
Let me know If I can be of assistance solving the merge conflicts. I was the one modifying those files.
Sorry this has taken a while, I've got quite a bit going on right now.
I've changed to a standard class, removed the decorator, and made the other changes requested. All tests seem are passing, including ssh_tools, so hopefully you get the same results on your side. I've also added more docstrings and comments so hopefully all is more clear now?
Hello Samuel,
thank you very much for the modifications. I will look deeper into it later.
Sorry this has taken a while, I've got quite a bit going on right now.
No need to say sorry. Really! Open source shouldn't be about saying and/or feeling sorry or explaining why something took longer than expected. This is unhealthy. Don't push yourself. Yes, open source is also about responsibility but not only for the project and their users but also for yourself. Open source should be fun, not pressure! ❤️
Relax. 😃
Regards, Christian
OK, this looks nice. Check careful my changes and/or test them. I might have broken something. 😆
Sure thing 😁
I modified the handling of the profile_id in BackupStatus class. I also tweaked
_human()a bit.I really love recursion. But I am not sure if we need it to print a simple two level dictionary. What do you think? The code is hard to read for outsiders.
Yes, when I started writing this, I had no idea how much information would be displayed or if more would be added over time so thought I was "future proofing" by using a recursive approach. Now that the requirements are more clear, it does seem quite unneccessary so I will make a simpler solution 👌
Despite of that I would say this is nearly finished. What do you think?
It feels like we're getting there 😁
Despite of that I would say this is nearly finished. What do you think?
It feels like we're getting there 😁
:)
Is this a "ready to merge" or should I wait for more to come?
I have removed the checking for successful backups based on date, simplified the human readable output, removed the indent frmo json, and removed the issues flag (no longer relevant without checking for backups with errors). Linting passes on my machine with pylint, ruff, and flake8 (with --verbose and -v flags) but have skipped ci so you can check from your side if you like. Please let me know if you would like any other changes.
FYI: Writing it myself.
EDIT: OK, I updated the man page and added a rudimentary section in the manual.
Hello Samuel, regarding this PR, can you please have a look in my comment in another issue.
https://github.com/bit-team/backintime/issues/2296#issuecomment-3522722124
Your "--status" is involved and I have no idea how to solve it. It need to be fixed until the next release (1.6.0) around spring 2026. Otherwise I need to disable the "--status" feature for a while until we improve our understanding and control over this mounting-stuff in BIT.
Regards, Christian