borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
12
62
Fork 49
Code Issues 49 Pull Requests 1 Actions Packages Releases 132 Activity

CONFIG_PATHS no longer accepts multiple values #919

New Issue
Closed
opened 2024-10-01 22:24:35 +00:00 by holocronweaver ยท 6 comments
holocronweaver commented 2024-10-01 22:24:35 +00:00
Contributor
Copy Link

What I'm trying to do and why

I upgraded from borgmatic 1.7.13 to 1.8.14, and now this command with two configs no longer works:

/borgmatic -c server.yaml critical.yaml -v 2

Steps to reproduce

Try the following in borgmatic 1.7.13 and 1.8.14 (should not matter if the config files even exist, though I verified mine are valid by running them individually): borgmatic -c server.yaml critical.yaml -v 2

Actual behavior

$ borgmatic -c server.yaml critical.yaml -v 2
usage: borgmatic [-h] [-c CONFIG_PATHS] [-n] [-nc] [-v {-2,-1,0,1,2}] [--syslog-verbosity {-2,-1,0,1,2}]
                 [--log-file-verbosity {-2,-1,0,1,2}] [--monitoring-verbosity {-2,-1,0,1,2}]
                 [--log-file LOG_FILE] [--log-file-format LOG_FILE_FORMAT] [--log-json]
                 [--override OPTION.SUBOPTION=VALUE] [--no-environment-interpolation] [--bash-completion]
                 [--fish-completion] [--version]
                 ...
Unrecognized argument: critical.yaml

Need some help? https://torsion.org/borgmatic/#issues

Expected behavior

The command to run successfully as it did in 1.7.13.

Other notes / implementation ideas

The latest --help for the -c flag example seems to be the same in both versions I tested (still space separated):

-c CONFIG_PATHS, --config CONFIG_PATHS
                        Configuration filename or directory, can specify flag multiple times, defaults to:
                        /etc/borgmatic/config.yaml /etc/borgmatic.d $HOME/.config/borgmatic/config.yaml
                        $HOME/.config/borgmatic.d

I think I've identified the root cause.

In 1.7.13 --config had nargs='*': https://projects.torsion.org/borgmatic-collective/borgmatic/src/tag/1.7.13/borgmatic/commands/arguments.py#L131

But at some point nargs was removed:

borgmatic/commands/arguments.py Lines 298 to 304 in f54d566edc
global_group.add_argument(
'-c',
'--config',
dest='config_paths',
action='append',
help=f"Configuration filename or directory, can specify flag multiple times, defaults to: {' '.join(unexpanded_config_paths)}",
)

I verified that treating this as a per action=append without nargs works in 1.8.14:

borgmatic -c server.yaml -c critical.yaml -v 2
/home/me/scripts/backup/borg/server.yaml: No commands to run for pre-everything hook
/home/me/scripts/backup/borg/critical.yaml: Running command for pre-everything hook
...

If this is an intentional change, then please update the documentation example with something like: '-c '.join(default_config_paths):

-c CONFIG_PATHS, --config CONFIG_PATHS
                        Configuration filename or directory, can specify flag multiple times, defaults to:
                        -c /etc/borgmatic/config.yaml /etc/borgmatic.d  -c $HOME/.config/borgmatic/config.yaml -c $HOME/.config/borgmatic.d

and consider restoring the original nargs since this is such a central argument to the command so removing it will likely cause quite a bit of noise when backup scripts unexpectedly need to be updated for a minor version change.

(Minor rant: I argue that apps with semantic versioning should avoid breaking changes outside of major version bumps, e.g. v1 -> v2 can break but not v1.1 -> v1.2. I know not all apps follow this, but at least in the Python-verse my understanding is minor versions are usually backwards-compatible, which makes me wonder if this change wasn't meant to be released in v1: https://packaging.python.org/en/latest/discussions/versioning/#semantic-versioning)

borgmatic version

1.8.14

borgmatic installation method

pip install borgmatic

Borg version

1.2.1

Python version

3.10.12

Database version (if applicable)

n/a

Operating system and version

Ubuntu 22.04.4 server

### What I'm trying to do and why I upgraded from borgmatic 1.7.13 to 1.8.14, and now this command with two configs no longer works: ``` /borgmatic -c server.yaml critical.yaml -v 2 ``` ### Steps to reproduce Try the following in borgmatic 1.7.13 and 1.8.14 (should not matter if the config files even exist, though I verified mine are valid by running them individually): `borgmatic -c server.yaml critical.yaml -v 2` ### Actual behavior ``` $ borgmatic -c server.yaml critical.yaml -v 2 usage: borgmatic [-h] [-c CONFIG_PATHS] [-n] [-nc] [-v {-2,-1,0,1,2}] [--syslog-verbosity {-2,-1,0,1,2}] [--log-file-verbosity {-2,-1,0,1,2}] [--monitoring-verbosity {-2,-1,0,1,2}] [--log-file LOG_FILE] [--log-file-format LOG_FILE_FORMAT] [--log-json] [--override OPTION.SUBOPTION=VALUE] [--no-environment-interpolation] [--bash-completion] [--fish-completion] [--version] ... Unrecognized argument: critical.yaml Need some help? https://torsion.org/borgmatic/#issues ``` ### Expected behavior The command to run successfully as it did in `1.7.13`. ### Other notes / implementation ideas The latest `--help` for the `-c` flag example seems to be the same in both versions I tested (still space separated): ``` -c CONFIG_PATHS, --config CONFIG_PATHS Configuration filename or directory, can specify flag multiple times, defaults to: /etc/borgmatic/config.yaml /etc/borgmatic.d $HOME/.config/borgmatic/config.yaml $HOME/.config/borgmatic.d ``` I think I've identified the root cause. In 1.7.13 `--config` had `nargs='*'`: https://projects.torsion.org/borgmatic-collective/borgmatic/src/tag/1.7.13/borgmatic/commands/arguments.py#L131 But at some point `nargs` was removed: https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/f54d566edcaf1b9f13d7596b26c45a3dfdf5b40a/borgmatic/commands/arguments.py#L298-L304 I verified that treating this as a per `action=append` without `nargs` works in 1.8.14: ``` borgmatic -c server.yaml -c critical.yaml -v 2 /home/me/scripts/backup/borg/server.yaml: No commands to run for pre-everything hook /home/me/scripts/backup/borg/critical.yaml: Running command for pre-everything hook ... ``` If this is an intentional change, then please update the documentation example with something like: `'-c '.join(default_config_paths)`: ``` -c CONFIG_PATHS, --config CONFIG_PATHS Configuration filename or directory, can specify flag multiple times, defaults to: -c /etc/borgmatic/config.yaml /etc/borgmatic.d -c $HOME/.config/borgmatic/config.yaml -c $HOME/.config/borgmatic.d ``` and consider restoring the original `nargs` since this is such a central argument to the command so removing it will likely cause quite a bit of noise when backup scripts unexpectedly need to be updated for a minor version change. (Minor rant: I argue that apps with semantic versioning should avoid breaking changes outside of major version bumps, e.g. v1 -> v2 can break but not v1.1 -> v1.2. I know not all apps follow this, but at least in the Python-verse my understanding is minor versions are usually backwards-compatible, which makes me wonder if this change wasn't meant to be released in v1: https://packaging.python.org/en/latest/discussions/versioning/#semantic-versioning) ### borgmatic version 1.8.14 ### borgmatic installation method pip install borgmatic ### Borg version 1.2.1 ### Python version 3.10.12 ### Database version (if applicable) n/a ### Operating system and version Ubuntu 22.04.4 server
witten commented 2024-10-02 15:46:02 +00:00
Owner
Copy Link

Thanks for taking the time to file this and track down the cause! This is indeed an intentional change. Here's the changelog from 1.8.0 that describes it:

  • BREAKING: Flags like "--config" that previously took multiple values now need to be given once per value, e.g. "--config first.yaml --config second.yaml" instead of "--config first.yaml second.yaml". This prevents argument parsing errors on ambiguous commands.

What I can do though is go ahead and update the command-line documentation as per your suggestion. I think that better clarifies the change.

To your point about breaking changes, borgmatic does not follow traditional semantic versioning because: 1. It would lead to version number bloat, and 2. It's too restrictive. However, it's not a total wild west; here is how borgmatic versioning generally works:

  • Super major breaking changes with major version bumps: 1, 2, etc.
  • Breaking changes with minor version bumps: 1.7, 1.8, etc.
  • Minor breaking changes (e.g. a bug fix that's technically a breaking change that only affects a small subset of users) with patch version bumps: 1.8.14, 1.8.15, etc.

I should probably publish that "policy" somewhere...

Thanks for taking the time to file this and track down the cause! This is indeed an intentional change. Here's the changelog from 1.8.0 that describes it: > * BREAKING: Flags like "--config" that previously took multiple values now need to be given once per value, e.g. "--config first.yaml --config second.yaml" instead of "--config first.yaml second.yaml". This prevents argument parsing errors on ambiguous commands. What I can do though is go ahead and update the command-line documentation as per your suggestion. I think that better clarifies the change. To your point about breaking changes, borgmatic does not follow traditional semantic versioning because: 1. It would lead to version number bloat, and 2. It's too restrictive. However, it's not a total wild west; here is how borgmatic versioning generally works: * Super major breaking changes with major version bumps: 1, 2, etc. * Breaking changes with minor version bumps: 1.7, 1.8, etc. * Minor breaking changes (e.g. a bug fix that's technically a breaking change that only affects a small subset of users) with patch version bumps: 1.8.14, 1.8.15, etc. I should probably publish that "policy" somewhere...
witten commented 2024-10-02 15:48:31 +00:00
Owner
Copy Link

Implemented the suggested documentation fix in main. Thank you!

Implemented the suggested documentation fix in main. Thank you!
๐ŸŽ‰ 1
witten commented 2024-10-02 16:21:20 +00:00
Owner
Copy Link

Versioning policy published! https://torsion.org/borgmatic/docs/how-to/upgrade/#versioning-and-breaking-changes

Versioning policy published! https://torsion.org/borgmatic/docs/how-to/upgrade/#versioning-and-breaking-changes
๐Ÿ‘ 1
holocronweaver commented 2024-10-02 20:59:41 +00:00
Author
Contributor
Copy Link

Thanks so much! Even when I disagree with you, you are always a pleasure to work with. ๐Ÿ˜„

Thanks so much! Even when I disagree with you, you are always a pleasure to work with. ๐Ÿ˜„
witten commented 2024-10-02 21:38:35 +00:00
Owner
Copy Link

Hah, that's the best I can hope for. Thank you!

Hah, that's the best I can hope for. Thank you!
witten commented 2024-11-03 21:34:37 +00:00
Owner
Copy Link

Released in borgmatic 1.9.0!

Released in borgmatic 1.9.0!
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#919
No description provided.
Delete Branch "%!s()"

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?