Fail fast for invalid RESTIC_PACK_SIZE env values#5592
Conversation
What does this PR change? What problem does it solve? - previously a typo like RESTIC_PACK_SIZE=64MiB let a backup run to completion while silently falling back to the default pack size, so you only learn much later that the wrong pack size was used; now we return a fatal error as soon as the env var can’t be parsed, matching the CLI flag’s validation - document the rationale inline so future readers know why we guard the env path - add tests that cover both the failure case (64MiB) and a valid numeric env value Was the change previously discussed in an issue or on the forum? - small bugfix, no prior discussion Checklist - [x] I have added tests for all code changes - [ ] I have added documentation for relevant changes (not needed) - [ ] There's a new file in changelog/unreleased/ (not needed for this small bugfix) - [x] I'm done! This pull request is ready for review Tests - GOCACHE=$(pwd)/.gocache go test ./internal/global
|
For context, what I really wanted to do was to refactor all environment variable parsers to lean on Cobra/ |
- remember the raw RESTIC_PACK_SIZE value and parse it during PreRun only when the user did not also pass --pack-size, so CLI overrides keep working while still failing fast for env typos - ignore PreRun entirely for Cobra’s __complete/__completeNoDesc helpers so shell completion keeps working even if a global env var is invalid - add regression tests that cover the failure, success, and “env invalid but flag valid” cases
|
Seeing the discussion in #4759 just now. I’ve updated this PR so RESTIC_PACK_SIZE is parsed during PreRun (and skipped for the completion helpers), then ignored whenever --pack-size was provided. That way we still fail fast for real commands, but autocompletion and CLI overrides continue to work. We could also add tests for the completion helper (by calling Cobra __complete). It seems to me that is something we're missing. Implementing it looks doable but it's a bigger change. |
MichaelEischer
left a comment
There was a problem hiding this comment.
Thanks! Please add a short changelog entry of type "Bugfix", then this PR can be merged.
Seeing the discussion in #4759 just now.
The refactoring I've mentioned there is completed by now. What is remaining is a proper pattern for more easily handling and validating environment variables.
For context, what I really wanted to do was to refactor all environment variable parsers to lean on Cobra/pflag so that the same parsing/validation is used for both CLI flags and environment variables.
That sounds like you already have something specific in mind?
What does this PR change? What problem does it solve?
Was the change previously discussed in an issue or on the forum?
Checklist
Tests