invalid environment variable values silently ignored

[eharris]

Output of restic version

restic 0.16.4 (v0.16.4-0-g3786536dc) compiled with go1.21.8 on linux/amd64

What backend/service did you use to store the repository?

local/any

Problem description / Steps to reproduce

restic seems to silently ignore problems with some environment variable parameters.
In my instance, the documentation wasn't clear that RESTIC_PACK_SIZE needed to be numeric only (and was in MB) and it silently ignored RESTIC_PACK_SIZE=64M and continued to use the default rather than issuing a warning or failing.
It does properly warn if the problem parameter is on the command line.

Expected behavior

I would expect restic to warn or fail if an environment variable it is using is not valid (as long as it is not being overridden on the command line).

Actual behavior

restic silently ignores problems with environment variables like RESTIC_PACK_SIZE.

Did restic help you today? Did it make you happy in any way?

Using it in a new situation, working pretty well!

[MichaelEischer]

This is unfortunately rather complex to properly fix. Currently, environment variables are parsed inside init() functions before the regular execution starts. At that point, the code has no idea which command will be called in the end. Blindly failing if the environment variable value leads to a few weird corner cases:

• shell autocompletion will stop working if an invalid environment variable is set
• environment variables for a different command can cause the current one to fail
• RESTIC_PACK_SIZE=64M restic --pack-size 64 ... would still fail

Avoiding these corner cases, however, requires restructuring how all commands are defined. And I'm not yet sure how a correct implementation would look like.

[konidev20]

Hey @MichaelEischer
Do you think we can use something similar to what's present in the internal/backend/backend.go where there is an interface called ApplyEnvironmenter this interface has a method called ApplyEnvironment.

We enforce that all backend configurations must implement this method to read any required environment variables.

Do you think something similar can be done for the Command options, where we can enforce an order of reading environment variables, command options and a validate function?

[MichaelEischer]

Do you think something similar can be done for the Command options, where we can enforce an order of reading environment variables, command options and a validate function?

Adding something similar for the global options should be relatively easy. Extending that to more commands in a clean way isn't that easy however. While we could introduce some callback mess, I'd like to invert how the command setup works to get rid of all the involved global variables.

We currently have global variable with the options for each command along with another global variable that holds the cobra.Command. These are initialized in an init() function and therefore have no clean way of returning an error.

The clean way to implement this for the commands is to completely refactor the used pattern. The first part is that move the setup of the command flags into an Add(...) method on the options struct used by the command, see cmd/restic/cmd_key_add.go for an example. Next, the actual command creation also has to move into a NewKeyAddCmd function. The call then has to add the command to the parent command an so on. The call to New*Cmd could then also handle and verify the environment variables.

As a stopgap solution we could add a GlobalOptions.ApplyEnvironment() error method and call that from cmd/restic/main.go . That would at least take care of the problem here.