[simpleqt]

Root cause

BACKUP_STOP_DURING_BACKUP_LABEL has a non-empty default (true), so in Config.resolve() (cmd/backup/config.go:353) the check

if c.BackupStopDuringBackupLabel != "" && c.BackupStopContainerLabel != "" {
    err = errwrap.Wrap(nil, "both BACKUP_STOP_DURING_BACKUP_LABEL and BACKUP_STOP_CONTAINER_LABEL have been set, cannot continue")
    return
}

is always true when the deprecated BACKUP_STOP_CONTAINER_LABEL is set — no matter whether its successor was configured by the user or just filled in from its default. This makes the deprecation path below it unreachable:

• the run aborts with both ... have been set, cannot continue, and
• the documented warning ("The old key will stay supported until the next major version, but logs a warning each time a backup is taken.", docs/how-tos/replace-deprecated-backup-stop-container-label.md:19) is never logged, which is also why the reporter of cannot run when only setting BACKUP_STOP_CONTAINER_LABEL #882 never saw a deprecation message.

Reproduction

A container that only sets the deprecated key breaks all of its backup runs:

backup:
  image: offen/docker-volume-backup:latest
  environment:
    BACKUP_CRON_EXPRESSION: 0 4,16 * * *
    BACKUP_STOP_CONTAINER_LABEL: headscale

level=ERROR msg="Unexpected error running schedule 0 4,16 * * *: main.(*Config).resolve: both
BACKUP_STOP_DURING_BACKUP_LABEL and BACKUP_STOP_CONTAINER_LABEL have been set, cannot continue"

The same configuration works when BACKUP_STOP_DURING_BACKUP_LABEL="" is added, i.e. the conflict is only reported because of the default value.

Fix

loadConfig now records the keys that have actually been provided by the user (cmd/backup/config_provider.go), which allows resolve() to detect a conflict based on an explicitly set successor key only. The deprecated key is then transposed and warned about as documented, and configurations that really do set both keys are still rejected.

Test evidence

New test in cmd/backup/config_stop_label_test.go, run with go test ./cmd/backup/.

Before (HEAD):

=== RUN   TestDeprecatedBackupStopContainerLabel/deprecated_key_on_its_own
    config_stop_label_test.go:72: backup.(*Config).resolve: both BACKUP_STOP_DURING_BACKUP_LABEL and BACKUP_STOP_CONTAINER_LABEL have been set, cannot continue
--- FAIL: TestDeprecatedBackupStopContainerLabel (0.00s)
    --- FAIL: TestDeprecatedBackupStopContainerLabel/deprecated_key_on_its_own (0.00s)
FAIL	github.com/offen/docker-volume-backup/cmd/backup	0.944s

After:

=== RUN   TestDeprecatedBackupStopContainerLabel
    --- PASS: TestDeprecatedBackupStopContainerLabel/deprecated_key_on_its_own (0.00s)
    --- PASS: TestDeprecatedBackupStopContainerLabel/deprecated_key_next_to_explicitly_set_successor (0.00s)
    --- PASS: TestDeprecatedBackupStopContainerLabel/successor_key_on_its_own (0.00s)
    --- PASS: TestDeprecatedBackupStopContainerLabel/defaults (0.00s)
ok  	github.com/offen/docker-volume-backup/cmd/backup	0.049s

gofmt -l cmd internal reports no files.

Refs #882

Not run: the Docker based integration suite in test/ (no Docker available here), golangci-lint (not installed locally).

[m90]

Thanks for working on this. I have a feeling this should either be simpler (just checking the raw env) or more ergonomic (not having to know about the env to config mapping). Currently, the added complexity is not really worth it.

Let me know what you think.

[simpleqt]

You're right — the extra machinery wasn't worth it. I've replaced it with the raw-env check:

if c.BackupStopContainerLabel != "" {
    // BACKUP_STOP_DURING_BACKUP_LABEL defaults to true, so the parsed values
    // cannot tell whether the user has set it on purpose. Look at the
    // environment instead, the same way DOCKER_HOST is checked in script.go.
    if os.Getenv("BACKUP_STOP_DURING_BACKUP_LABEL") != "" {
        err = errwrap.Wrap(nil, "both BACKUP_STOP_DURING_BACKUP_LABEL and BACKUP_STOP_CONTAINER_LABEL have been set, cannot continue")
        return
    }
    ...

That dropped the provided map, the hasProvided helper and the whole loadConfig change — config_provider.go is now untouched and the diff is two files: config.go (+11/−4) and the new test. The presence check in script.go (_, dockerHostSet := os.LookupEnv("DOCKER_HOST")) was the precedent I followed; os.Getenv here also treats an explicitly empty value as unset, which matches the old != "" intent.

Tests: the subtest that exercises the conflict now sets the key with t.Setenv as well, since the check reads the environment — go test -run TestDeprecatedBackupStopContainerLabel ./cmd/backup/ → 4/4 subtests pass, and the whole package is ok.

One consequence worth your call. Because the check reads the environment, a successor key supplied through the _FILE indirection (BACKUP_STOP_DURING_BACKUP_LABEL_FILE) is no longer seen. Before my change that combination was rejected; now it warns and line 364 (c.BackupStopDuringBackupLabel = c.BackupStopContainerLabel) overwrites the file-provided successor with the deprecated value. It's a narrow case, and I'd rather flag it than paper over it — if you want it covered, it needs the value comparison kept alongside this check, which is more code again. Happy to do it either way; my preference is to keep it simple as you suggested.

Also for completeness: RED at submission was --- FAIL: TestDeprecatedBackupStopContainerLabel/deprecated_key_on_its_own → config_stop_label_test.go:72: backup.(*Config).resolve: both BACKUP_STOP_DURING_BACKUP_LABEL and BACKUP_STOP_CONTAINER_LABEL have been set, cannot continue. I re-ran GREEN after this simplification (above), but my attempt to re-capture RED for the revised test stalled — the Go toolchain on this box hangs intermittently — so I'm not claiming a fresh RED run for it; CI will exercise it.

[m90]

It's a narrow case, and I'd rather flag it than paper over it

Considering this has been deprecated a long time ago, I think it's fine to keep it simple. Thanks for raising in any case.