config bootstrap CLI parsing issues #712

New Issue

Closed

opened 2023-06-16 16:54:33 +00:00 by witten · 4 comments

witten commented 2023-06-16 16:54:33 +00:00

Owner

Copy Link

What I'm trying to do and why

As part of developing another feature (#529), I found a few edge cases where the new config bootstrap action (#697) doesn't work quite as intended on the command-line.

First issue: Steps to reproduce

$ borgmatic -v 1 config bootstrap --repository 1.2.borg

First issue: Actual behavior

usage: borgmatic config [-h] {bootstrap} ...
borgmatic config: error: argument {bootstrap}: invalid choice: '1' (choose from 'bootstrap')
Error parsing arguments: /usr/bin/borgmatic -v 1 config bootstrap --repository 1.2.borg

First issue: Expected behavior

Successful bootstrap with verbosity level 1.

Second issue: Steps to reproduce

$ borgmatic -v 1 bootstrap --repository 1.2.borg

Second issue: Actual behavior

Bootstrapping config paths: /root/tmp/test.yaml

summary:
Bootstrap successful

Second issue: Expected behavior

Error about unknown bootstrap action or similar, as the parent config action is missing.

Other notes / implementation ideas

Some of this might be a side effect of the bootstrap argument parser "living" at the top level alongside the other parsers.

Environment

borgmatic version: main (these issues do not occur in any released versions)

borgmatic installation method: pip editable

Borg version: 1.2.4

Python version: 3.10.10

Database version (if applicable): n/a

operating system and version: Manjaro stable

#### What I'm trying to do and why As part of developing another feature (#529), I found a few edge cases where the new `config bootstrap` action (#697) doesn't work quite as intended on the command-line. #### First issue: Steps to reproduce ``` $ borgmatic -v 1 config bootstrap --repository 1.2.borg ``` #### First issue: Actual behavior ``` usage: borgmatic config [-h] {bootstrap} ... borgmatic config: error: argument {bootstrap}: invalid choice: '1' (choose from 'bootstrap') Error parsing arguments: /usr/bin/borgmatic -v 1 config bootstrap --repository 1.2.borg ``` #### First issue: Expected behavior Successful bootstrap with verbosity level 1. #### Second issue: Steps to reproduce ``` $ borgmatic -v 1 bootstrap --repository 1.2.borg ``` #### Second issue: Actual behavior ``` Bootstrapping config paths: /root/tmp/test.yaml summary: Bootstrap successful ``` #### Second issue: Expected behavior Error about unknown `bootstrap` action or similar, as the parent `config` action is missing. #### Other notes / implementation ideas Some of this might be a side effect of the `bootstrap` argument parser "living" at the top level alongside the other parsers. #### Environment **borgmatic version:** main (these issues do not occur in any released versions) **borgmatic installation method:** pip editable **Borg version:** 1.2.4 **Python version:** 3.10.10 **Database version (if applicable):** n/a **operating system and version:** Manjaro stable

witten added the

bug

label 2023-06-16 16:54:40 +00:00

witten commented 2023-06-19 21:33:55 +00:00

Author

Owner

Copy Link

I have a fix in hand. I may have, uh, majorly refactored all of argument parsing though as part of this. Currently working on tests....

I have a fix in hand. I may have, uh, majorly refactored all of argument parsing though as part of this. Currently working on tests....

👀 1

witten referenced this issue from a commit 2023-06-19 23:19:50 +00:00

Refactor arguments parsing to fix bootstrap action CLI issues (#712).

witten commented 2023-06-19 23:36:37 +00:00

Author

Owner

Copy Link

Done and pushed. High-level, even before #697, argument parsing was kind of a complexity farm, having gotten that way through gradual accretion. So it wasn't a great foundation upon which to build the #697 config bootstrap feature. As part of fixing this ticket, I did a refactor of argument parsing both to support the bootstrap use cases and all the other random argument parsing use cases that code needed to support. I think the end result is still quite complex and more than a little brittle, but some of the incidental complexity has been stripped away.

Summary of changes:

• Get rid of the multiple loops over arguments and repeated parse_known_args() calls. There's a single pass now.
• Instead of merging sub-parsers and sub-sub-parsers (which caused some of the issues in this ticket), now sub-actions are parsed as part of parsing actions.
• I standardized the terminology in the arguments parsing, so it's almost all now in terms of "actions" and "sub-actions".
• Much of the argument parsing has been broken out to individually tested utility functions where possible.
• More integration tests to cover many of the sub-action use cases and edge cases, including ones from this ticket.
• Probably a bunch of stuff I'm forgetting.

I want to emphasize that this is largely me cleaning up long-standing tech debt in this area of code while still trying to incorporate and support your changes from #697.

Done and pushed. High-level, even before #697, argument parsing was kind of a complexity farm, having gotten that way through gradual accretion. So it wasn't a great foundation upon which to build the #697 `config bootstrap` feature. As part of fixing this ticket, I did a refactor of argument parsing both to support the `bootstrap` use cases and all the other random argument parsing use cases that code needed to support. I think the end result is still quite complex and more than a little brittle, but _some_ of the incidental complexity has been stripped away. Summary of changes: * Get rid of the multiple loops over arguments and repeated `parse_known_args()` calls. There's a single pass now. * Instead of merging sub-parsers and sub-sub-parsers (which caused some of the issues in this ticket), now sub-actions are parsed as part of parsing actions. * I standardized the terminology in the arguments parsing, so it's almost all now in terms of "actions" and "sub-actions". * Much of the argument parsing has been broken out to individually tested utility functions where possible. * More integration tests to cover many of the sub-action use cases and edge cases, including ones from this ticket. * Probably a bunch of stuff I'm forgetting. I want to emphasize that this is largely me cleaning up long-standing tech debt in this area of code while still trying to incorporate and support your changes from #697.

witten closed this issue 2023-06-19 23:36:37 +00:00

witten reopened this issue 2023-06-21 17:20:03 +00:00

witten commented 2023-06-21 17:24:21 +00:00

Author

Owner

Copy Link

Found another couple...

3. Steps to reproduce

$ borgmatic config bootstrap --repository 1.2.borg -v 1

3. Actual behavior

usage: borgmatic config [-h] {bootstrap} ...
borgmatic config: error: argument {bootstrap}: invalid choice: '1' (choose from 'bootstrap')
Error parsing arguments: /usr/bin/borgmatic config bootstrap --repository 1.2.borg -v 1

3. Expected behavior

Successful bootstrap with verbosity level 1.

...

4. Steps to reproduce

$ borgmatic -c test.yaml config bootstrap --repository 1.2.borg

4. Actual behavior

summary:
/root/tmp/config: Error parsing configuration file
[Errno 2] No such file or directory: '/root/tmp/config'
Bootstrap successful

4. Expected behavior

The config action is not treated as a second configuration file, the actual configuration file is ignored, and the bootstrap proceeds as per normal.

Found another couple... #### 3. Steps to reproduce ``` $ borgmatic config bootstrap --repository 1.2.borg -v 1 ``` #### 3. Actual behavior ``` usage: borgmatic config [-h] {bootstrap} ... borgmatic config: error: argument {bootstrap}: invalid choice: '1' (choose from 'bootstrap') Error parsing arguments: /usr/bin/borgmatic config bootstrap --repository 1.2.borg -v 1 ``` #### 3. Expected behavior Successful bootstrap with verbosity level 1. ... #### 4. Steps to reproduce ``` $ borgmatic -c test.yaml config bootstrap --repository 1.2.borg ``` #### 4. Actual behavior ``` summary: /root/tmp/config: Error parsing configuration file [Errno 2] No such file or directory: '/root/tmp/config' Bootstrap successful ``` #### 4. Expected behavior The `config` action is not treated as a second configuration file, the actual configuration file is ignored, and the bootstrap proceeds as per normal.

witten referenced this issue from a commit 2023-06-21 17:41:58 +00:00

Fix for another subaction argument-parsing edge case (#712).

witten referenced this issue from a commit 2023-06-21 17:48:11 +00:00

Add a test for another edge case (#712).

witten closed this issue 2023-06-21 17:48:17 +00:00

witten commented 2023-06-25 05:31:19 +00:00

Author

Owner

Copy Link

Just released in borgmatic 1.7.15.

Just released in borgmatic 1.7.15.

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

spot-check

1.8.10

1.8.9

1.8.8

1.8.7

1.8.6

1.8.5

1.8.4

1.8.3

1.8.2

1.8.1

1.8.0

1.7.15

1.7.14

1.7.13

1.7.12

1.7.11

1.7.10

1.7.9

1.7.8

1.7.7

1.7.6

1.7.5

1.7.4

1.7.3

1.7.2

1.7.1

1.7.0

1.6.6

1.6.5

1.6.4

1.6.3

1.6.2

1.6.1

1.6.0

1.5.24

1.5.23

1.5.22

1.5.21

1.5.20

1.5.19

1.5.18

1.5.17

1.5.16

1.5.15

1.5.14

1.5.13

1.5.12

1.5.11

1.5.10

1.5.9

1.5.8

1.5.7

1.5.7.dev0

1.5.6

1.5.5

1.5.4

1.5.3

1.5.2

1.5.1

1.5.0

1.4.22

1.4.21

1.4.20

1.4.19

1.4.18

1.4.17

1.4.16

1.4.15

1.4.14

1.4.13

1.4.12

1.4.11

1.4.10

1.4.9

1.4.8

1.4.7

1.4.6

1.4.5

1.4.4

1.4.3

1.4.2

1.4.1

1.4.0

1.3.26

1.3.25

1.3.24

1.3.23

1.3.22

1.3.21

1.3.20

1.3.19

1.3.18

1.3.17

1.3.16

1.3.15

1.3.14

1.3.13

1.3.12

1.3.11

1.3.10

1.3.9

1.3.8

1.3.7

1.3.6

1.3.5

1.3.4

1.3.3

1.3.2

1.3.1

1.3.0

1.2.18

1.2.17

1.2.16

1.2.15

1.2.14

1.2.13

1.2.12

1.2.11

1.2.10

1.2.9

1.2.8

1.2.7

1.2.6

1.2.5

1.2.4

1.2.3

1.2.2

1.2.1

1.2.0

1.1.15

1.1.14

1.1.13

1.1.12

1.1.11

1.1.10

1.1.9

1.1.7

1.1.6

1.1.5

1.1.4

1.1.3

1.1.2

1.1.1

1.1.0

1.0.3

1.0.2

1.0.1

1.0.0

0.1.8

0.1.7

0.1.6

0.1.5

0.1.4

0.1.3

0.1.2

0.1.1

0.1.0

0.0.7

0.0.6

0.0.5

0.0.4

0.0.3

0.0.2

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

1 Participants

Notifications

Subscribe

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#712

No description provided.