borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
15
61
Fork 46
Code Issues 58 Pull Requests 2 Actions Packages Releases 125 Activity

WIP: Added debug log entries for borg, borgmatic and os version #750

Closed
IBims1NicerTobi wants to merge 2 commits from (deleted):main into main
pull from: (deleted):main
merge into: borgmatic-collective:main
Conversation 7 Commits 2 Files Changed 2 +45
IBims1NicerTobi commented 2023-09-02 17:28:22 +00:00
Contributor
Copy Link

This implements changes discussed in #714. This currently break unit and integration tests for borgmatic as it generates additional log entries.

This implements changes discussed in #714. This currently break unit and integration tests for borgmatic as it generates additional log entries.
IBims1NicerTobi added 1 commit 2023-09-02 17:28:22 +00:00
IBims1NicerTobi added 1 commit 2023-09-02 17:28:35 +00:00
witten reviewed 2023-09-06 05:09:07 +00:00
witten left a comment
Owner
Copy Link

This all looks super useful, and the general approach you've taken makes sense to me. Besides fixing the broken (and possibly brittle) tests, I'd like to see some test coverage for this code, even if it's just a few basic "does not raise" tests to exercise the various code branches. That'd be unit tests if you do mocking, integration if you don't.

Beyond that, I see a few more items to log from the ticket (#714) that you could include if you saw value in them. Were you planning on including any of them or were gonna reduce scope?

This all looks super useful, and the general approach you've taken makes sense to me. Besides fixing the broken (and possibly brittle) tests, I'd like to see some test coverage for _this_ code, even if it's just a few basic "does not raise" tests to exercise the various code branches. That'd be unit tests if you do mocking, integration if you don't. Beyond that, I see a few more items to log from the ticket (#714) that you could include if you saw value in them. Were you planning on including any of them or were gonna reduce scope?
borgmatic/commands/borgmatic.py
@ -73,6 +74,7 @@ def run_configuration(config_filename, config, arguments):
try:
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-09-06 05:02:28 +00:00
Owner
Copy Link

Is there any reason this is done here rather than passing local_borg_version to log_versions()? That would ensure it'd get logged in the same spot as all of your other nice debug log statements.

Is there any reason this is done here rather than passing `local_borg_version` to `log_versions()`? That would ensure it'd get logged in the same spot as all of your other nice debug log statements.
IBims1NicerTobi commented 2023-09-06 09:57:17 +00:00
Author
Contributor
Copy Link

The borg version depends on the config file and run_configuration() is called later than log_versions(). Also there is the possibility of having one borg version for each config file, so we cannot really easily collect that in log_versions().

The borg version depends on the config file and `run_configuration()` is called later than `log_versions()`. Also there is the possibility of having one borg version for each config file, so we cannot really easily collect that in `log_versions()`.
witten commented 2023-09-06 17:13:56 +00:00
Owner
Copy Link

Makes sense! Thanks for the explanation.

Makes sense! Thanks for the explanation.
borgmatic/debug_env.py
@ -0,0 +14,4 @@
'''
Logs the platform and the architecture on debug. Also logs the version of borgmatic and python.
'''
logging.debug(f'Borgmatic version: {importlib_metadata.version("borgmatic")}')
witten commented 2023-09-06 04:50:32 +00:00
Owner
Copy Link

borgmatic is always lowercase! (It's a convention often followed by command-line Unix tools.)

borgmatic is always lowercase! (It's a convention often followed by command-line Unix tools.)
IBims1NicerTobi commented 2023-09-06 10:01:07 +00:00
Author
Contributor
Copy Link

Will be fixed

Will be fixed
borgmatic/debug_env.py
@ -0,0 +28,4 @@
elif 'NAME' in os_release:
logging.debug(f'Distribution name: {os_release["NAME"]}')
if 'VERSION' in os_release:
logging.debug(f'Distribution version: {os_release["VERSION"]}')
witten commented 2023-09-06 04:56:48 +00:00
Owner
Copy Link

What do you think of just logging the following when 'NAME' in os_release:?

logging.debug('Distribution: {os_release["NAME"]} {os_release.get("VERSION", "")}')

That'd be more comparable to the PRETTY_NAME case.

What do you think of just logging the following when `'NAME' in os_release:`? ```python logging.debug('Distribution: {os_release["NAME"]} {os_release.get("VERSION", "")}') ``` That'd be more comparable to the `PRETTY_NAME` case.
IBims1NicerTobi commented 2023-09-06 10:01:15 +00:00
Author
Contributor
Copy Link

lgtm

lgtm
IBims1NicerTobi commented 2023-09-06 10:15:31 +00:00
Author
Contributor
Copy Link
  • Version of borgmatic, borg, and Python itself
    Basically done
  • Install path for the above (to help identify install method)
    Not really something I see value in (the install method should not really matter much when you have the version)
  • OS name and version
    Done
  • Database version
    Per hook implementation and depends on the database
    MariaDB for example supports SELECT VERSION();
  • Maybe version of grafana loki (I wrote that hook and it should be easy to extend to also get the server version of loki)
  • User that is running borgmatic
    Will be implemented soon
  • Ownership info of borgmatic config folder
    Will probably be a different pull request as this might be a rather extensive feature
    Also better error messages for missing file/wrong permissions is something I am looking at
- Version of borgmatic, borg, and Python itself Basically done - Install path for the above (to help identify install method) Not really something I see value in (the install method should not really matter much when you have the version) - OS name and version Done - Database version Per hook implementation and depends on the database MariaDB for example supports `SELECT VERSION();` - Maybe version of grafana loki (I wrote that hook and it should be easy to extend to also get the server version of loki) - User that is running borgmatic Will be implemented soon - Ownership info of borgmatic config folder Will probably be a different pull request as this might be a rather extensive feature Also better error messages for missing file/wrong permissions is something I am looking at
witten commented 2023-09-06 17:23:58 +00:00
Owner
Copy Link

That all looks good. The main value in knowing the borgmatic install method is when debugging issues related to the installation. For instance, running a crontab when borgmatic is installed into a virtualenv is different than running a crontab when borgmatic comes from a system package.

That all looks good. The main value in knowing the borgmatic install method is when debugging issues related to the installation. For instance, [running a crontab when borgmatic is installed into a virtualenv](https://projects.torsion.org/borgmatic-collective/borgmatic/issues/752) is different than running a crontab when borgmatic comes from a system package.
IBims1NicerTobi closed this pull request 2023-11-17 22:05:35 +00:00
witten commented 2023-11-17 22:24:11 +00:00
Owner
Copy Link

Just curious.. Did you decide not to finish this one after all?

Just curious.. Did you decide not to finish this one after all?
IBims1NicerTobi commented 2023-11-18 00:16:22 +00:00
Author
Contributor
Copy Link

No I just realized how much code the fixing of the tests will be. I am currently rewriting it since it is outdated anyways and needs more work. I will probably merge 1-2 lines of logging code and 100 lines of unit tests (In a new pull request).

No I just realized how much code the fixing of the tests will be. I am currently rewriting it since it is outdated anyways and needs more work. I will probably merge 1-2 lines of logging code and 100 lines of unit tests (In a new pull request).
👍 1
IBims1NicerTobi commented 2023-11-18 00:18:20 +00:00
Author
Contributor
Copy Link

Also since I have some time (around 1 month) between jobs and university I will try to add a podman volume plugin too since I could very well use that.

Also since I have some time (around 1 month) between jobs and university I will try to add a podman volume plugin too since I could very well use that.
👍 1
IBims1NicerTobi commented 2023-11-18 00:33:31 +00:00
Author
Contributor
Copy Link

#792 is the new pull request. So far that one is only for borg versions but I want to take a step at a time.

#792 is the new pull request. So far that one is only for borg versions but I want to take a step at a time.
witten commented 2023-11-23 06:51:09 +00:00
Owner
Copy Link

Sounds good, thanks!

Sounds good, thanks!

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#750
No description provided.
Delete Branch "(deleted):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?