Add --validate action: Validate borgmatic configuration file and exit #158

ypid · 2019-05-05T20:48:33Z

ypid commented

2019-05-05 20:48:33 +00:00

Useful when generating the borgmatic configuration file with configuration management and before moving the generated file in place checking if it is actually valid.

The added unit tests are currently incomplete. Can you help me with those? How should I pass a valid and invalid configuration to the command?

PS: Thanks again very much for maintaining borgmatic. I contributed some time ago already and still have the feeling that borgmatic is in pretty good shape 👍

Useful when generating the borgmatic configuration file with configuration management and before moving the generated file in place checking if it is actually valid. The added unit tests are currently incomplete. Can you help me with those? How should I pass a valid and invalid configuration to the command? PS: Thanks again very much for maintaining borgmatic. I contributed some time ago already and still have the feeling that borgmatic is in pretty good shape :+1:

witten commented

2019-05-07 22:19:52 +00:00

Thank you for doing this! Great idea for a feature.. I do the same config-management-driven borgmatic config generation thing on at least one of my hosts.

What do you think of making this feature an entirely separate validate-borgmatic-config command-line command rather than part of the main borgmatic command? I suggest that for a couple of reasons:

That'd require less branching logic within the main borgmatic command.
I don't think there are any use cases where you'd want to use, say, --validate and --create simultaneously. In fact, if a user did that with your current branch, they might be surprised that no archives got created! So a separate validation-only command might have less surprising behavior
A separate validation command could be written with very little duplication given that parse_configuration() is already factored out of borgmatic.py. (And side note: You wouldn't need to do any of the cross-file validation checks.)
Automated tests may be easier to write since there's less moving parts to test in a separate command. Alternatively, given that the surface area of a separate validation command would be so tiny, it may not need tests for its main() function at all. (Even borgmatic.py's main() is not under test, you'll notice.)

Let me know what you think!

Thank you for doing this! Great idea for a feature.. I do the same config-management-driven borgmatic config generation thing on at least one of my hosts. What do you think of making this feature an entirely separate `validate-borgmatic-config` command-line command rather than part of the main `borgmatic` command? I suggest that for a couple of reasons: * That'd require less branching logic within the main borgmatic command. * I don't *think* there are any use cases where you'd want to use, say, `--validate` and `--create` simultaneously. In fact, if a user did that with your current branch, they might be surprised that no archives got created! So a separate validation-only command might have less surprising behavior * A separate validation command could be written with very little duplication given that `parse_configuration()` is already factored out of `borgmatic.py`. (And side note: You wouldn't need to do any of the cross-file validation checks.) * Automated tests *may* be easier to write since there's less moving parts to test in a separate command. Alternatively, given that the surface area of a separate validation command would be so tiny, it may not need tests for its `main()` function at all. (Even `borgmatic.py`'s `main()` is not under test, you'll notice.) Let me know what you think!

👍 1

decentral1se approved these changes 2019-05-08 08:31:47 +00:00

decentral1se left a comment

Thanks for this good work! Just passing by and thought I would pitch in with a review (not really familiar with the code, so feel free to ignore).

borgmatic/commands/borgmatic.py

						
				@ -48,2 +48,4 @@

				    actions_group = parser.add_argument_group('actions')

				    actions_group.add_argument(

				        '-V', '--validate', dest='validate', action='store_true', help='Validate borgmatic configuration file and exit'

decentral1se commented

2019-05-08 08:27:46 +00:00

Can drop the and exit?

Can drop the ` and exit`?

ypid commented

2019-05-09 21:09:15 +00:00

--version has the same. Also note:

I don’t think there are any use cases where you’d want to use, say, --validate and --create simultaneously.

`--version` has the same. Also note: > I don’t think there are any use cases where you’d want to use, say, --validate and --create simultaneously.

borgmatic/commands/borgmatic.py

						
				@ -484,4 +489,9 @@ def main():  # pragma: no cover

				    [logger.handle(log) for log in summary_logs if log.levelno >= logger.getEffectiveLevel()]

				    if any(log.levelno == logging.CRITICAL for log in summary_logs):

				        if args.validate:

decentral1se commented

2019-05-08 08:30:02 +00:00

This is never triggered because the above section where logging.CRITICAL is outputted is now not run?

This is never triggered because the above section where `logging.CRITICAL` is outputted is now not run?

ypid commented

2019-05-09 21:12:46 +00:00

Thanks for the input. I am actually unsure why I wanted to skip exit_with_help_link().

Thanks for the input. I am actually unsure why I wanted to skip `exit_with_help_link()`.

borgmatic/commands/borgmatic.py

						
				@ -487,1 +494,4 @@

				        exit_with_help_link()

				    if args.validate:

				        print('Configuration files are valid.')

decentral1se commented

2019-05-08 08:30:58 +00:00

Should also logging.makeLogRecord or?

Nitpicking but don't see any other user facing messages with . either.

Should also `logging.makeLogRecord` or? Nitpicking but don't see any other user facing messages with `.` either.

decentral1se commented

2019-05-08 08:32:45 +00:00

Uhhh, I clicked the wrong button ... not approving, just wanted to comment! ;)

ypid commented

2019-05-09 22:16:05 +00:00

Thanks for your feedback!

@witten: I agree with your points. Please have a look at witten/borgmatic#161. Feel free to do the finishing touches yourself. That might be more efficient for us.

Thanks for your feedback! @witten: I agree with your points. Please have a look at https://projects.torsion.org/witten/borgmatic/pulls/161. Feel free to do the finishing touches yourself. That might be more efficient for us.

witten commented

2019-05-10 19:50:23 +00:00

Closing this in favor of #161. Thanks again for taking the time to do both.

witten closed this pull request

2019-05-10 19:50:23 +00:00

the build failed

Details