refactor: centralize process configuration as a single source of truth - #2035
Merged
Conversation
- Introduce domain config packages and Config.Load as the sole production resolver - Split flags into FlagSpec domains with Viper bind and CLI-neutral env bridging - Build complete UpdateParams once for run-once, schedule, and HTTP API paths - Drain RunConfig, startup messaging, and API options assembly out of cmd - Point notifications at resolved config.Notify instead of scraping Cobra flags
- Reverse firstNonEmpty argument order to prefer Compatibility over Client - Update TestClientOptionsMapsClientAndCompat to assert new precedence rule - Add fallback assertion for empty Compatibility CPUCopyMode value
- Update firstEnv to skip empty values for non-presence-meaning-true keys - Restrict formatEnvForFlag empty-value handling to NO_COLOR only - Add test verifying empty WATCHTOWER_CLEANUP and WATCHTOWER_DEBUG are ignored while NO_COLOR presence enables no-color
- Extract IsPresenceEnvKey and hasPresenceEnvKey helpers from presenceEnvKeys map - Rename presenceEmptyEnvKeys to presenceEnvKeys and clarify documentation - Skip BindEnv for presence-based keys to prevent Viper from re-parsing "0"/"false" - Remove runtime NO_COLOR lookup from logging/register.go, use static false default - Add test verifying NO_COLOR presence enables flag for all values including empty, 0, and false
- Delete RegisterFromSpecs and registerOne helpers - Remove unused utils import for flag deprecation marking - Eliminate redundant static default flag registration path in favor of BindEnv-only approach
- Ensure params.Filter and the positional argument to runUpdatesWithNotifications remain identical - Prefer explicit schedule filter over BaseParams filter to prevent divergent sources
- Collect non-presence env aliases before invoking BindEnv - Use single variadic call per flag to maintain key ordering - Continue skipping presence-only keys for ApplyEnvToFlags
- Clarify all domains expose FlagSpec with static pflag defaults - Document BindAll and Load resolve flag > env > default precedence - Explain ApplyEnvToFlags bridges env without baking into registration defaults
- Verify WATCHTOWER_TIMEOUT maps to stop-timeout as bare seconds - Verify WATCHTOWER_NOTIFICATION_URL splits into notification-url entries - Confirm ApplyEnvToFlags preserves Changed semantics for CLI overrides
- Move comprehensive URL parsing cases to internal/flags/utils/listparse_test.go - Replace broad test with focused env-to-flag wiring verification - Remove unused utils import from flags_test.go
… conversion - Add DurationFromSeconds to centralize float64-to-time.Duration clamping logic - Replace inline overflow checks in EnvDuration with reusable helper - Update apply_env to use clamped conversion for bare-second duration values
…RunUpgradesOnSchedule - Eliminate intermediate variables that duplicate ScheduleDeps fields - Access deps fields directly to simplify the function body - Reduce unnecessary variable declarations and shadowing
…ge caller - Remove redundant noStartupMessage parameter from SetupStartupLogger - Consolidate suppression check in WriteStartupMessage before logger setup - Drop unused notifications package import - Update test coverage for simplified helper interface
…leInfo - Embed ScheduleInfo struct in StartupParams to replace flat fields - Remove RunOnce, UpdateOnStart, HTTPAPI, and Sched from StartupParams definition - Update config.StartupParams builder and logging tests for new struct layout
…erplate - Add testDeps function returning ScheduleDeps with common test defaults - Replace inline ScheduleDeps literals with testDeps calls in test cases - Allow selective field overrides for specific test scenarios
…scheduleBase variable - Rename apiBase to sharedBase to reflect its use across HTTP API and schedule paths - Remove redundant scheduleBase variable and duplicate skipSelfUpdate conditional - Use single sharedBase instance for both BaseParams assignments
- Add testScheduleDeps factory function providing default ScheduleDeps for scheduling tests - Replace verbose inline ScheduleDeps literals in three TestUpdateOnStart* cases with helper calls - Remove redundant nil and empty field assignments now supplied by helper defaults
…fier - Update test description to specify standard logger entry return value - Add assertion verifying underlying logger is logrus.StandardLogger() - Document suppression handling delegation in test comment
- Replace raw URL logging with count to avoid exposing embedded tokens - Add trace-level logging for full URL inspection when needed
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThis PR centralizes CLI and environment configuration into typed snapshots, then rewires command execution, actions, scheduling, HTTP API routes, logging, and notification construction to consume those snapshots and structured dependency objects. ChangesConfig-driven architecture refactor
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 22 medium |
🟢 Metrics 222 complexity · -234 duplication
Metric Results Complexity 222 Duplication -234
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/notifications/notifier_test.go (1)
120-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest doesn't actually exercise "delay and legacy delay both defined" precedence.
delaySecondsis passed as0here, so this is functionally identical to the "legacy delay is defined" case above it — it never proves that legacy delay wins over a real configured delay.✅ Proposed fix
- delay := notifications.GetDelay(0, 7*time.Second) + delay := notifications.GetDelay(3, 7*time.Second) gomega.Expect(delay).To(gomega.Equal(7 * time.Second))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/notifier_test.go` around lines 120 - 130, The legacy delay precedence test currently passes 0 for delaySeconds, so it does not cover both values being configured. Update the GetDelay call in the “legacy delay and delay is defined” test to pass a non-zero delaySeconds distinct from the legacy delay, while keeping the expectation that the legacy delay value (7 seconds) is selected.pkg/notifications/slack.go (1)
96-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: Internal
Do not log raw Slack/Discord webhook credentials at Debug level.
pkg/notifications/slack.go:97logss.HookURL, and both generated service URLs (Discord at line 122 and Slack at line 152) include webhook tokens/credentials in their query strings. Redact these values or log them only with an unavailable/fallback log format, not as structured debug fields. Same concern also applies to the GoGotify service URL atpkg/notifications/gotify.go:175.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/slack.go` around lines 96 - 98, The GetURL implementations for Slack, Discord, and Gotify must not log webhook URLs or credentials. Remove raw URL values from structured debug fields and use a redacted or unavailable fallback value while preserving the existing URL-generation behavior.pkg/notifications/msteams.go (1)
82-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: Internal
Move credential-bearing legacy notifier GetURL logs to
Trace. The per-typeGetURLmethods continue to log the generated Shoutrrr URL atDebug, which can expose secrets enabled by the configured log level.
pkg/notifications/msteams.go:82-105: logsn.webHookURLand the generatedservice_url; use redacted or trace-only logging, especially for the Teams bearer URL.pkg/notifications/email.go:169-174:conf.GetURL().String()can includeuser:passin the SMTP URL; move it behindTrace.pkg/notifications/gotify.go:173-177:service_urlincludesgotifyAppToken; move it behindTrace.pkg/notifications/slack.go:97-152: logshook_urlandservice_url, both containing Slack/Discord webhook tokens; move them behindTrace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/msteams.go` around lines 82 - 106, Move credential-bearing URL logging from Debug to Trace in msTeamsTypeNotifier.GetURL and the affected sites: pkg/notifications/msteams.go:82-106, change webhook and generated service URL logs; pkg/notifications/email.go:169-174, change conf.GetURL().String() logging; pkg/notifications/gotify.go:173-177, change service_url logging; and pkg/notifications/slack.go:96-153, change hook_url and service_url logging. Preserve the existing messages and URL generation behavior while ensuring these secret-containing values are only emitted at Trace level.
🧹 Nitpick comments (8)
pkg/notifications/notifier.go (3)
125-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
notifyFromFlagsexceeds length guideline (66 LOC vs 50 limit).Flagged by static analysis. Since it's a test/deprecated-path helper, consider splitting legacy-field extraction into a smaller helper to ease future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/notifier.go` around lines 125 - 193, Reduce notifyFromFlags below the 50-line guideline by extracting the legacy notification flag reads and notifyConfig.Legacy construction into a focused helper, then reuse that helper from notifyFromFlags. Keep the existing flag names and resulting Notify values unchanged, with the change centered on notifyFromFlags and the new legacy-extraction helper.Source: Linters/SAST tools
63-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDelay precedence logic duplicated three times.
The legacy-delay-vs-configured-delay precedence check at lines 66-69 duplicates
GetDelay(lines 368-391), andAppendLegacyUrls(342-352) repeats it a third time. Consider reusingGetDelayhere to avoid future divergence.♻️ Proposed fix
urls, delay := appendLegacyURLs(urls, cfg.LegacyTypes, cfg.Legacy) - - // Prefer legacy delay when set; otherwise use the configured delay in seconds. - if delay == 0 && cfg.DelaySeconds > 0 { - delay = time.Duration(cfg.DelaySeconds) * time.Second - } + delay = GetDelay(cfg.DelaySeconds, delay)And similarly in
AppendLegacyUrls:- urls, legacyDelay := appendLegacyURLs(urls, cfg.LegacyTypes, cfg.Legacy) - - if legacyDelay == 0 && cfg.DelaySeconds > 0 { - return urls, time.Duration(cfg.DelaySeconds) * time.Second - } - - return urls, legacyDelay + urls, legacyDelay := appendLegacyURLs(urls, cfg.LegacyTypes, cfg.Legacy) + + return urls, GetDelay(cfg.DelaySeconds, legacyDelay)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/notifier.go` around lines 63 - 69, Reuse the existing GetDelay logic for delay precedence in the notifier initialization flow instead of duplicating the delay == 0 and cfg.DelaySeconds check. Update AppendLegacyUrls as well to delegate to GetDelay, ensuring legacy delay remains preferred and configured seconds remain the fallback.
258-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
appendLegacyURLsexceeds length/complexity guidelines (53 LOC, cyclomatic complexity 9).Flagged by static analysis (limits: 50 LOC, complexity 8). Consider extracting the per-type
switchinto a small constructor lookup/map to reduce branching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/notifier.go` around lines 258 - 324, Reduce the length and cyclomatic complexity of appendLegacyURLs by replacing its per-notification-type switch with a constructor lookup map or equivalent helper. Preserve shoutrrr skipping, unknown-type handling, notifier URL generation, delay extraction, and existing logging behavior while keeping appendLegacyURLs focused on iteration and processing.Source: Linters/SAST tools
internal/scheduling/scheduling.go (1)
58-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Startupand the sibling scalar fields overlap, and the scalars silently win.
Startup logging.StartupParamsalready carriesFiltering,Scope,Client,Notifier, andVersion, but lines 262-267 unconditionally overwrite them fromFilterDesc/Scope/Client/Notifier/MetaVersion. A caller that populatesStartup.Version(and leavesMetaVersionempty) gets it silently cleared. Consider keeping one source of truth — either drop the duplicated scalars, or document thatStartuponly suppliesNoStartupMessage/ScheduleInfo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/scheduling.go` around lines 58 - 95, The scheduling startup configuration has duplicate sources of truth, with scalar fields overriding values already present in ScheduleDeps.Startup. Update the startup-message construction flow around ScheduleDeps and the code assigning Filtering, Scope, Client, Notifier, and Version so Startup values are preserved; either remove the duplicated scalar dependencies and use Startup consistently, or explicitly constrain Startup to only NoStartupMessage and ScheduleInfo while ensuring callers cannot have its overlapping fields silently cleared.internal/config/load.go (1)
214-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a normalize-loop helper to fix the length warning.
Codacy flags
loadFilterat 67 lines (limit 50), driven by three near-identical "get slice → normalize each element" blocks.♻️ Proposed refactor
+// normalizeEach applies fn to every element of list in place and returns it. +func normalizeEach(list []string, fn func(string) string) []string { + for i := range list { + list[i] = fn(list[i]) + } + + return list +} + func loadFilter(vip *viper.Viper, flagSet *pflag.FlagSet, args []string) (filter.Filter, error) { labelEnable := vip.GetBool("label-enable") disableContainers := stringSliceValue( vip, flagSet, "disable-containers", []string{"WATCHTOWER_DISABLE_CONTAINERS"}, spec.ListCommaOrSpace, ) - for i := range disableContainers { - disableContainers[i] = util.NormalizeContainerName(disableContainers[i]) - } + disableContainers = normalizeEach(disableContainers, util.NormalizeContainerName) monitorImages := stringSliceValue( vip, flagSet, "monitor-image-names", []string{"WATCHTOWER_MONITOR_IMAGE_NAMES"}, spec.ListCommaOrSpace, ) - for i := range monitorImages { - monitorImages[i] = strings.TrimSpace(monitorImages[i]) - } + monitorImages = normalizeEach(monitorImages, strings.TrimSpace) skipImages := stringSliceValue( vip, flagSet, "skip-image-names", []string{"WATCHTOWER_SKIP_IMAGE_NAMES"}, spec.ListCommaOrSpace, ) - for i := range skipImages { - skipImages[i] = strings.TrimSpace(skipImages[i]) - } + skipImages = normalizeEach(skipImages, strings.TrimSpace)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/load.go` around lines 214 - 289, Extract the repeated slice-loading and element-normalization logic from loadFilter into a small helper, then use it for disableContainers, monitorImages, and skipImages while preserving their existing configuration keys, environment variables, parsing modes, and normalization functions. Keep label-based values and the rest of loadFilter unchanged.Source: Linters/SAST tools
internal/config/viper_get.go (1)
74-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
bareSecondsduplicatesutils.DurationFromSeconds.Same clamp logic as
internal/flags/utils/env.goLines 62-74. Reuse the helper to keep overflow behavior in one place.♻️ Proposed refactor
func bareSeconds(raw string) time.Duration { val, err := strconv.ParseFloat(raw, 64) if err != nil { return 0 } - nanos := val * float64(time.Second) - - if nanos > float64(math.MaxInt64) { - return time.Duration(math.MaxInt64) - } - - if nanos < float64(math.MinInt64) { - return time.Duration(math.MinInt64) - } - - return time.Duration(nanos) + return utils.DurationFromSeconds(val) }(drop the now-unused
mathimport)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/viper_get.go` around lines 74 - 92, Update bareSeconds to delegate numeric parsing and duration conversion to the existing utils.DurationFromSeconds helper, removing its duplicated ParseFloat and clamp logic; then remove the now-unused math import while preserving bareSeconds’s zero result for invalid input.internal/flags/apply_env.go (1)
204-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne
ListParseKind→ splitter mapping copied into two packages. Both functions have byte-identical switch bodies; a new list-parse strategy must be added in two places or env and Load resolution silently diverge.
internal/flags/apply_env.go#L204-L216: deleteparseEnvListand call the shared helper (e.g.utils.ParseList(raw, parse)).internal/config/viper_get.go#L144-L156: deleteparseListand call the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flags/apply_env.go` around lines 204 - 216, The list-parse strategy mapping is duplicated across parseEnvList and parseList, so new strategies can diverge. In internal/flags/apply_env.go:204-216, remove parseEnvList and use the shared utils.ParseList(raw, parse) helper; in internal/config/viper_get.go:144-156, remove parseList and make the same shared-helper call, preserving existing ListParseKind behavior.internal/flags/spec/register.go (1)
47-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting per-kind register helpers to reduce complexity.
registerOne's cyclomatic complexity (18, flagged by Codacy) stems from the shorthand/no-shorthand branch repeated perFlagKind, plus duplicated nil-default handling betweenKindStringSlice/KindStringArray. Extracting one small helper per kind (or a kind→registrar map) would flatten this and remove the duplication.♻️ Sketch of a per-kind extraction
+func registerBool(flagSet *pflag.FlagSet, s FlagSpec) { + def, _ := s.Default.(bool) + if s.Shorthand != "" { + flagSet.BoolP(s.Name, s.Shorthand, def, s.Help) + } else { + flagSet.Bool(s.Name, def, s.Help) + } +} + +func stringSliceDefault(v any) []string { + def, _ := v.([]string) + if def == nil { + def = []string{} + } + return def +} + func registerOne(flagSet *pflag.FlagSet, flagSpec FlagSpec) error { switch flagSpec.Kind { case KindBool: - def, _ := flagSpec.Default.(bool) - if flagSpec.Shorthand != "" { - flagSet.BoolP(flagSpec.Name, flagSpec.Shorthand, def, flagSpec.Help) - } else { - flagSet.Bool(flagSpec.Name, def, flagSpec.Help) - } + registerBool(flagSet, flagSpec) // ... similarly for other kinds, using stringSliceDefault for slice/array cases🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flags/spec/register.go` around lines 47 - 115, Refactor registerOne so each FlagKind’s registration logic is moved into dedicated per-kind helpers or an equivalent kind-to-registrar dispatch, eliminating the repeated shorthand/no-shorthand branches. Consolidate the shared nil-to-empty default handling for KindStringSlice and KindStringArray, while preserving existing defaults, help text, shorthand behavior, deprecation handling, hidden-flag handling, and unsupported-kind errors.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/notify-upgrade.go`:
- Around line 71-87: The flag set decorated before loading configuration is
inconsistent with the one consumed by appconfig.Load. Update the notify-upgrade
flow around flagSet, flags.ApplyEnvToFlags, flags.ProcessFlagAliases, and
flags.GetSecretsFromFiles so it uses cmd.PersistentFlags(), matching Load’s
binding source and preserving environment, alias, secret, and inherited CLI
values.
In `@internal/config/timeout_test.go`:
- Around line 39-49: Make the parse-error path for huge bare numeric values in
durationValue consistent with other invalid environment values by using the
existing pflag/default duration fallback instead of returning 0. Update the
“unparseable huge integer treated as zero bare seconds” expectation accordingly,
while preserving the overflow clamping behavior covered by the neighboring test.
In `@internal/flags/notify/register.go`:
- Around line 99-106: Correct the Help text for the "notifications" flag in the
flag registration so the opening bracket is properly closed, while preserving
the existing description and legacy notification types.
In `@internal/flags/utils/listparse.go`:
- Around line 131-139: The invalid URL warning in the result-processing loop
should not expose the full notification URL. Update the logrus.Warnf call in the
URL parsing block to report the parse error without urlStr, while preserving the
existing validation and append behavior.
In `@internal/scheduling/scheduling.go`:
- Around line 189-203: Guard the `deps.RunUpdate` invocation in the scheduling
flow before calling it, including the `UpdateOnStart` path that runs outside
cron recovery. When the hook is nil, follow the existing
nil-fallback/error-handling convention used by `Lock`, `WriteStartupMessage`,
and `Filter` instead of dereferencing it; otherwise preserve the current
`updateFilter`, `params`, and metric behavior.
In `@pkg/notifications/notifier.go`:
- Around line 85-87: The notifier logging paths must never emit complete
Shoutrrr URLs or credentials, including the trace call around the notifier URL
load, appendLegacyURLs, and SMTP legacy URL construction. Remove URL-containing
fields from logs or route them through a consistent redaction helper that strips
query/auth tokens, usernames, passwords, and sensitive query parameters while
preserving non-sensitive context.
---
Outside diff comments:
In `@pkg/notifications/msteams.go`:
- Around line 82-106: Move credential-bearing URL logging from Debug to Trace in
msTeamsTypeNotifier.GetURL and the affected sites:
pkg/notifications/msteams.go:82-106, change webhook and generated service URL
logs; pkg/notifications/email.go:169-174, change conf.GetURL().String() logging;
pkg/notifications/gotify.go:173-177, change service_url logging; and
pkg/notifications/slack.go:96-153, change hook_url and service_url logging.
Preserve the existing messages and URL generation behavior while ensuring these
secret-containing values are only emitted at Trace level.
In `@pkg/notifications/notifier_test.go`:
- Around line 120-130: The legacy delay precedence test currently passes 0 for
delaySeconds, so it does not cover both values being configured. Update the
GetDelay call in the “legacy delay and delay is defined” test to pass a non-zero
delaySeconds distinct from the legacy delay, while keeping the expectation that
the legacy delay value (7 seconds) is selected.
In `@pkg/notifications/slack.go`:
- Around line 96-98: The GetURL implementations for Slack, Discord, and Gotify
must not log webhook URLs or credentials. Remove raw URL values from structured
debug fields and use a redacted or unavailable fallback value while preserving
the existing URL-generation behavior.
---
Nitpick comments:
In `@internal/config/load.go`:
- Around line 214-289: Extract the repeated slice-loading and
element-normalization logic from loadFilter into a small helper, then use it for
disableContainers, monitorImages, and skipImages while preserving their existing
configuration keys, environment variables, parsing modes, and normalization
functions. Keep label-based values and the rest of loadFilter unchanged.
In `@internal/config/viper_get.go`:
- Around line 74-92: Update bareSeconds to delegate numeric parsing and duration
conversion to the existing utils.DurationFromSeconds helper, removing its
duplicated ParseFloat and clamp logic; then remove the now-unused math import
while preserving bareSeconds’s zero result for invalid input.
In `@internal/flags/apply_env.go`:
- Around line 204-216: The list-parse strategy mapping is duplicated across
parseEnvList and parseList, so new strategies can diverge. In
internal/flags/apply_env.go:204-216, remove parseEnvList and use the shared
utils.ParseList(raw, parse) helper; in internal/config/viper_get.go:144-156,
remove parseList and make the same shared-helper call, preserving existing
ListParseKind behavior.
In `@internal/flags/spec/register.go`:
- Around line 47-115: Refactor registerOne so each FlagKind’s registration logic
is moved into dedicated per-kind helpers or an equivalent kind-to-registrar
dispatch, eliminating the repeated shorthand/no-shorthand branches. Consolidate
the shared nil-to-empty default handling for KindStringSlice and
KindStringArray, while preserving existing defaults, help text, shorthand
behavior, deprecation handling, hidden-flag handling, and unsupported-kind
errors.
In `@internal/scheduling/scheduling.go`:
- Around line 58-95: The scheduling startup configuration has duplicate sources
of truth, with scalar fields overriding values already present in
ScheduleDeps.Startup. Update the startup-message construction flow around
ScheduleDeps and the code assigning Filtering, Scope, Client, Notifier, and
Version so Startup values are preserved; either remove the duplicated scalar
dependencies and use Startup consistently, or explicitly constrain Startup to
only NoStartupMessage and ScheduleInfo while ensuring callers cannot have its
overlapping fields silently cleared.
In `@pkg/notifications/notifier.go`:
- Around line 125-193: Reduce notifyFromFlags below the 50-line guideline by
extracting the legacy notification flag reads and notifyConfig.Legacy
construction into a focused helper, then reuse that helper from notifyFromFlags.
Keep the existing flag names and resulting Notify values unchanged, with the
change centered on notifyFromFlags and the new legacy-extraction helper.
- Around line 63-69: Reuse the existing GetDelay logic for delay precedence in
the notifier initialization flow instead of duplicating the delay == 0 and
cfg.DelaySeconds check. Update AppendLegacyUrls as well to delegate to GetDelay,
ensuring legacy delay remains preferred and configured seconds remain the
fallback.
- Around line 258-324: Reduce the length and cyclomatic complexity of
appendLegacyURLs by replacing its per-notification-type switch with a
constructor lookup map or equivalent helper. Preserve shoutrrr skipping,
unknown-type handling, notifier URL generation, delay extraction, and existing
logging behavior while keeping appendLegacyURLs focused on iteration and
processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ba106b0-82b0-4a18-bd1e-1671db7443ad
📒 Files selected for processing (74)
cmd/notify-upgrade.gocmd/root.gocmd/root_test.gointernal/actions/actions.gointernal/actions/actions_test.gointernal/actions/doc.gointernal/api/api_integration_test.gointernal/api/config/config.gointernal/api/config/config_test.gointernal/api/lifecycle_test.gointernal/api/routes/check.gointernal/api/routes/check_test.gointernal/api/routes/config.gointernal/api/routes/containers.gointernal/api/routes/update.gointernal/api/routes/update_test.gointernal/config/api/api.gointernal/config/client/client.gointernal/config/client_opts.gointernal/config/compatibility/compatibility.gointernal/config/config.gointernal/config/doc.gointernal/config/docker/docker.gointernal/config/filter/filter.gointernal/config/lifecycle/lifecycle.gointernal/config/load.gointernal/config/logging/logging.gointernal/config/mode/mode.gointernal/config/notify/notify.gointernal/config/overrides.gointernal/config/precedence_test.gointernal/config/registry/registry.gointernal/config/run_config.gointernal/config/schedule/schedule.gointernal/config/timeout_test.gointernal/config/update/update.gointernal/config/update_params.gointernal/config/update_params_test.gointernal/config/viper_get.gointernal/flags/api/register.gointernal/flags/apply_env.gointernal/flags/bind.gointernal/flags/client/register.gointernal/flags/compat/register.gointernal/flags/doc.gointernal/flags/docker/register.gointernal/flags/filter/register.gointernal/flags/flags.gointernal/flags/flags_test.gointernal/flags/lifecycle/register.gointernal/flags/logging/register.gointernal/flags/mode/register.gointernal/flags/notify/register.gointernal/flags/register.gointernal/flags/registry/register.gointernal/flags/schedule/register.gointernal/flags/spec/register.gointernal/flags/spec/spec.gointernal/flags/update/register.gointernal/flags/utils/deprecate.gointernal/flags/utils/env.gointernal/flags/utils/listparse.gointernal/flags/utils/listparse_test.gointernal/logging/startup.gointernal/logging/startup_test.gointernal/scheduling/scheduling.gointernal/scheduling/scheduling_test.gopkg/notifications/doc.gopkg/notifications/email.gopkg/notifications/gotify.gopkg/notifications/msteams.gopkg/notifications/notifier.gopkg/notifications/notifier_test.gopkg/notifications/slack.go
- Replace cmd.Flags() with cmd.PersistentFlags() in runNotifyUpgradeE - Ensures env/alias bridging uses the same flag set as config.Load
- Change legacy delay parameter from 0 to 5 in test case - Ensures test validates behavior with non-zero legacy delay input
- Move SMTP URL logging from debug to trace level in email notifier - Move Gotify API and service URL logging to trace level to protect embedded app tokens - Move Microsoft Teams webhook and service URL logging to trace level - Move Slack hook and service URL logging to trace level to protect embedded webhook tokens
- Move legacy notification flag extraction from notifyFromFlags to legacyFromFlags - Accept *pflag.FlagSet parameter to decouple from cobra.Command dependency
- Extract GetDelay helper to eliminate duplicate delay selection logic - Update NewNotifier and AppendLegacyUrls to use centralized delay resolution - Maintain precedence: legacy delay over configured DelaySeconds
…egacy notifiers - Define legacyNotifierCtor function type for legacy notifier construction - Register legacy notifier constructors in centralized legacyNotifierCtors map - Simplify appendLegacyURLs by replacing switch statement with map lookup
…tion - Populate Startup.Filtering, Scope, Client, Notifier, and Version at call site - Remove redundant field assignment from RunUpgradesOnSchedule
- Introduce normalizedStringSlice to combine loading and per-element transforms - Replace inline normalization loops in loadFilter for container and image lists - Apply util.NormalizeContainerName and strings.TrimSpace via unified path
- Remove math import and inline nanosecond clamping from bareSeconds - Delegate to utils.DurationFromSeconds for centralized duration conversion - Add documentation comment for bareSeconds function
- Extract ParseList helper to internal/flags/spec package - Replace local parseList and parseEnvList with centralized implementation - Update viper and flag callers to use spec.ParseList
- Decompose registerOne into dedicated helpers per flag kind - Improve godoc comments for Register, MustRegister, and registerOne - Clarify that environment values are resolved post-parse
- bareSeconds returns error for invalid input instead of zero - durationValue breaks to pflag default when bareSeconds fails - Update test to expect default value for unparseable huge integers
- Close unclosed bracket in notifications flag description
- Remove raw URL strings from validation error output to avoid exposing embedded tokens - Switch to structured logging with `WithError` for invalid notification URLs
- Check if RunUpdate is configured before invocation - Log debug message and return early when hook is unset
- Add redactServiceURL and redactServiceURLs helper functions - Apply redaction across email, gotify, msteams, shoutrrr, and slack notifiers - Log token length instead of raw token in Gotify initialization - Replace full URL slice logging with count in legacy URL appending
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2035 +/- ##
==========================================
- Coverage 77.71% 72.25% -5.46%
==========================================
Files 94 120 +26
Lines 12431 13316 +885
==========================================
- Hits 9661 9622 -39
- Misses 2392 3274 +882
- Partials 378 420 +42
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
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
1 participant
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.
This PR reworks how Watchtower loads and applies process configuration so flags, environment variables, and runtime policy resolve through one path instead of scattered Cobra/Viper reads.
Problem
Configuration and update policy were assembled in multiple places (cmd, flags, API, schedule, notifications). That made behavior hard to reason about, allowed partial UpdateParams builders, and mixed registration-time env baking with ad hoc flag Gets.
Solution
Introduce domain config packages and a single Config.Load resolver, split flags into FlagSpec domains with static defaults and consistent env bridging, and project one complete UpdateParams snapshot for run-once, schedule, and HTTP API. Drain orchestration assembly out of cmd and point notifications at resolved settings.
Changes
Summary by CodeRabbit