borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
10
66
Fork 30
Code Issues 46 Pull Requests 5 Actions Packages Releases 144 Activity

Added debug message that logs borg version for every config #792

Merged
witten merged 2 commits from :main into main 2023-11-25 03:59:40 +00:00
Conversation 12 Commits 2 Files Changed 1 +1
IBims1NicerTobi commented 2023-11-18 00:32:08 +00:00
Contributor
Copy Link
No description provided.
IBims1NicerTobi commented 2023-11-21 06:46:21 +00:00
Author
Contributor
Copy Link

@witten are you not interested in merging this yet or did you not get around to look at it?

@witten are you not interested in merging this yet or did you not get around to look at it?
witten commented 2023-11-21 06:48:30 +00:00
Owner
Copy Link

Did not get around to looking yet, sorry! I'm kind of underwater on borgmatic stuff at the moment. Barely managed to squeeze out a release. I'll have a look at this hopefully this week.

Did not get around to looking yet, sorry! I'm kind of underwater on borgmatic stuff at the moment. Barely managed to squeeze out a release. I'll have a look at this hopefully this week.
witten reviewed 2023-11-23 06:50:35 +00:00
borgmatic/commands/borgmatic.py Outdated
@ -77,1 +77,4 @@
local_borg_version = borg_version.local_borg_version(config, local_path)
yield from log_error_records(
f'{config_filename}: Local Borg version: {local_borg_version}', levelno=logging.DEBUG
)
witten commented 2023-11-23 06:50:17 +00:00
Owner
Copy Link

This should probably just be a logger.debug() or logger.info() call since it's not an error...? Unfortunately though such a change would obsolete some of your laborious test work. But the good news is it might actually make your test changes easier to mock.

Let me know your thoughts.

This should probably just be a `logger.debug()` or `logger.info()` call since it's not an error...? Unfortunately though such a change would obsolete some of your laborious test work. But the good news is it might actually make your test changes easier to mock. Let me know your thoughts.
IBims1NicerTobi commented 2023-11-23 10:28:33 +00:00
Author
Contributor
Copy Link

Yes that does indeed make a lot of sense and explains why the unit tests were such a pain.

Yes that does indeed make a lot of sense and explains why the unit tests were such a pain.
IBims1NicerTobi commented 2023-11-23 10:46:48 +00:00
Author
Contributor
Copy Link

Fixed now

Fixed now
IBims1NicerTobi force-pushed main from a20e1e95d8 to 8b9abc6cf8 2023-11-23 10:44:18 +00:00 Compare
IBims1NicerTobi added 1 commit 2023-11-23 10:46:18 +00:00
IBims1NicerTobi commented 2023-11-23 10:54:21 +00:00
Author
Contributor
Copy Link

Another question that's still up for debate is when to log all the versions. In the old pull request I did it in run_configuration() but that implies that we log everything multiple times since (I assume) for multiple configs this gets called for each one. Since the python version, borgmatic version and os should never change between config files it would probably be better to log them closer to startup than that so they only get logged once instead.

Another question that's still up for debate is when to log all the versions. In the old pull request I did it in `run_configuration()` but that implies that we log everything multiple times since (I assume) for multiple configs this gets called for each one. Since the python version, borgmatic version and os should never change between config files it would probably be better to log them closer to startup than that so they only get logged once instead.
witten commented 2023-11-23 16:45:21 +00:00
Owner
Copy Link

Since the python version, borgmatic version and os should never change between config files it would probably be better to log them closer to startup than that so they only get logged once instead.

Yes, that makes sense to me! I think it's fine if not everything is logged in the exact same place—some globally and some per-config as appropriate.

> Since the python version, borgmatic version and os should never change between config files it would probably be better to log them closer to startup than that so they only get logged once instead. Yes, that makes sense to me! I think it's fine if not everything is logged in the exact same place—some globally and some per-config as appropriate.
witten reviewed 2023-11-23 16:46:41 +00:00
witten left a comment
Owner
Copy Link

Looks good! I think it does make sense to log this here because it could vary between configuration files if the local path option varies. But I'd recommend capitalizing Borg here because convention.

Looks good! I think it does make sense to log this here because it *could* vary between configuration files if the local path option varies. But I'd recommend capitalizing Borg here because convention.
IBims1NicerTobi commented 2023-11-24 17:30:11 +00:00
Author
Contributor
Copy Link

What convention? borg is normally always written as borg not Borg, at least I have never seen it written that way anywhere?

What convention? borg is normally always written as `borg` not `Borg`, at least I have never seen it written that way anywhere?
witten commented 2023-11-24 17:45:14 +00:00
Owner
Copy Link

The convention at least in borgmatic documentation and error messages is to call it "Borg" (as per the Borg docs). borgmatic, on the other hand, is always lowercase!

The convention at least in borgmatic documentation and error messages is to call it "Borg" (as per the [Borg docs](https://www.borgbackup.org/)). borgmatic, on the other hand, is always lowercase!
IBims1NicerTobi added 1 commit 2023-11-24 17:47:48 +00:00
IBims1NicerTobi commented 2023-11-24 17:48:05 +00:00
Author
Contributor
Copy Link

Fixed now

Fixed now
witten commented 2023-11-24 21:22:56 +00:00
Owner
Copy Link

Thank you! Did you want me to merge just this change or did you want to do the rest of the ticket in this PR?

Thank you! Did you want me to merge just this change or did you want to do the rest of the ticket in this PR?
IBims1NicerTobi commented 2023-11-24 23:17:07 +00:00
Author
Contributor
Copy Link

Just this
I will do all the information separately later and I think there is a lot of room for discussion on how and were to implement the rest.

Just this I will do all the information separately later and I think there is a lot of room for discussion on how and were to implement the rest.
witten commented 2023-11-25 03:56:16 +00:00
Owner
Copy Link

Sounds good!

Sounds good!
witten merged commit e120dff9ff into main 2023-11-25 03:59:40 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
diivi import_bot witten
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#792
No description provided.
Delete Branch ":main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?