Repository navigation
Keep --env overrides across watch and server config reloads - #1625
Merged
Merged
Conversation
--env is applied only inside GetConfigFromCli (override process env, LoadConfig, restore env). The watch loop, scheduled watch ticks and the API server reload config with plain LoadConfig, so every watch iteration (even the first) silently dropped --env, e.g. --env=REMOTE_STORAGE=ftp fell back to the config file value, and `server --env` was never applied to the server config at all. Extract LoadConfigFromCli (returns error instead of log.Fatal, restores env even on load error, serialized by a mutex because overrides go through the process-global environment while API handlers reload concurrently) and use it in all reload sites. disable_environment_override semantics are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up to review of the --env reload fix: - /backup/actions and resume-after-restart re-enter cliApp with only -c and --command-id, so they ran with a different config than api.config once server --env was applied; prepend server --env so the command's own --env still wins. - server startup retried config load errors forever, now a bad --env value would hang startup silently; invalid config is fatal like in every other command, only the ClickHouse connection is awaited. - duplicated --env key recorded the first override as the "old" value, so restore left it in the long-lived server/watch process env. - reloads logged override K=V at info on every API call / iteration and left the log level reset to info after a load error; log once, then debug, and restore the previous level on error. - ReloadConfig error log now shows the resolved config path. Tests: TestLoadConfigFromCli snapshots ambient env, covers duplicated keys and log level; new TestServerKeepsEnvOverride pins ReloadConfig and cliArgs; TestWatchEnvOverride polls a background watch with a bounded deadline instead of a fixed timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-applying --env on each reload needs a temporary os.Setenv/restore window, and kelseyhightower/envconfig has no custom lookup to avoid it, so goroutines reading the environment concurrently could see or miss the overrides. Keep --env in the process environment after GetConfigFromCli instead: watch/server reloads go back to plain LoadConfig and see it, and /backup/actions plus resume-after-restart re-entries inherit server --env without extra args. Re-entered commands (--command-id set) still restore the environment, so --env of a single /backup/actions command can't leak into the long-lived server process. server loads config once via GetConfigFromCli, so a bad config or --env is fatal instead of retried forever. This drops LoadConfigFromCli, its mutex, log-once and cliArgs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Report for CI Build 39483697392Coverage at 70.32% (no base build to compare)Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
# Conflicts: # ChangeLog.md
# Conflicts: # ChangeLog.md
# Conflicts: # ChangeLog.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
--env KEY=VALUEoverrides were applied only insideGetConfigFromCliand immediately restored.watch(interval and--schedule) reloads the config with plainLoadConfigat the start of every iteration, so even the first backup ran without the overrides — e.g.watch --env REMOTE_STORAGE=ftpfailed withremote_storage shall not be "none", andbackups_to_keep_remotefrom--envwas ignored.server --env ...was never applied to the server's own config at all (startup,/restart, SIGHUP, API handlers). Found while writing the TestFlows test for #1535.Change
GetConfigFromCliapplies--envto the process environment once and keeps it for the life of the process; later reloads see the overrides naturally. No temporary set/restore window on reload (which would race with other goroutines reading the environment).--command-id != -1, e.g./backup/actions) restore the environment after loading, so a per-request--envdoesn't stick in the server.serverloads its config once viaGetConfigFromClibefore waiting for ClickHouse: an invalid config or--envvalue now exits instead of retrying forever; the 5s retry only waits for the ClickHouse connection./backup/actions, resume after restart) inherit the server's--env; a command's own--envstill wins.disable_environment_overrideis respected; for duplicate keys the last value wins.Tests
TestGetConfigFromCliKeepsEnvOverride(overrides survive laterLoadConfigreloads, server-run restore,disable_environment_override);go test -race ./pkg/config/... ./pkg/backup/... ./pkg/server/...pass.TestWatchEnvOverride(runswatch --env S3_PATH=...in background, polls for ≥2 backups under the--envpath, none under the config path) andTestServerAPIpass.🤖 Generated with Claude Code