fix(notify-upgrade): address command execution bugs and update migration docs - #2054
Merged
nicholas-fedor merged 2 commits intoJul 28, 2026
Merged
Conversation
…ion docs - Use Root().PersistentFlags for proper flag binding across command hierarchy - Initialize logging via SetupLogging prior to config resolution - Extract URL building into BuildURLs to avoid unnecessary notifier creation - Create temporary output file in working directory - Ensure output file includes trailing newline with Fprintln - Add mountinfo-based container ID detection as fallback mechanism - Overhaul migration tool documentation with comprehensive usage examples
3 tasks
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesNotification migration
Sequence Diagram(s)sequenceDiagram
participant NotifyUpgrade
participant RootConfig
participant BuildURLs
participant TempFile
NotifyUpgrade->>RootConfig: load root flags, environment, aliases, and secrets
RootConfig-->>NotifyUpgrade: cfg.Notify
NotifyUpgrade->>BuildURLs: build notification URLs
BuildURLs-->>NotifyUpgrade: Shoutrrr URL list
NotifyUpgrade->>TempFile: write generated environment file
Possibly related issues
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 |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 10 complexity · 0 duplication
Metric Results Complexity 10 Duplication 0
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.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2054 +/- ##
==========================================
- Coverage 72.25% 72.17% -0.08%
==========================================
Files 120 120
Lines 13318 13332 +14
==========================================
Hits 9623 9623
- Misses 3271 3285 +14
Partials 424 424
🚀 New features to boost your workflow:
|
![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: 5
🤖 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 157-166: Update the runtime command generation associated with
container ID resolution in cmd/notify-upgrade.go (lines 157-166) to emit the
valid two-operand Docker copy form, using the resolved container ID and file
path as the source and ./watchtower-notifications.env as the destination. Update
each corresponding example in
docs/notifications/deprecations/migration-tool/index.md at lines 208-225,
280-290, 377-387, 464-474, and 545-555 to remove duplicated container prefixes
and use the same valid docker cp syntax.
In `@docs/notifications/deprecations/migration-tool/index.md`:
- Around line 312-320: Update the Docker CLI example’s
WATCHTOWER_NOTIFICATION_URL assignment to quote the complete -e argument,
preserving the SMTP URL and all existing parameters while preventing shell
interpretation of its & separators.
- Around line 603-613: Update the temporary path lookup after starting the
container with docker compose run so it waits for notify-upgrade output, using
log polling or follow mode with a bounded timeout. In the FILE handling flow,
distinguish a container failure from delayed or missing output, while preserving
cleanup through the existing CONTAINER trap.
- Around line 128-166: The migration examples currently advertise Slack channel
configuration that notify-upgrade does not preserve. Update the conversion path
used by slackTypeNotifier.GetURL to retain the configured SlackChannel instead
of replacing it with "webhook"; alternatively, remove the Slack channel options
from the Docker Compose and CLI examples and document the limitation.
- Line 607: Update the documented FILE assignment to remove non-portable grep -P
usage, replacing it with portable extended-regex or shell-native extraction that
still selects the latest watchtower-notif-urls-* path from docker logs. Preserve
the existing fallback behavior when no match is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48338d19-1dea-41f0-847f-a416b4c0a005
📒 Files selected for processing (3)
cmd/notify-upgrade.godocs/notifications/deprecations/migration-tool/index.mdpkg/notifications/notifier.go
…ted slack channel - Fix log output to show docker cp instead of cp for container file extraction - Remove WATCHTOWER_NOTIFICATION_SLACK_CHANNEL from all configuration examples - Correct docker cp command syntax in documentation copy instructions - Add quotes around SMTP URL environment variable to preserve special characters
nicholas-fedor
deleted the
fix/2041-notify-upgrade-fixes-and-documentation-updates
branch
July 28, 2026 04:45
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.
The PR fixes the
notify-upgradesubcommand so it works correctly without relying on the root command's skippedPreRun, and expands the migration tool documentation with complete usage instructions.Problem
notify-upgradeis a Cobra subcommand and does not inheritPreRunfrom the root command. As a result, the command was missing critical setup: logging configuration was never applied, the current container ID was never resolved for copy instructions, and invalid legacy notification config caused a fatal exit instead of returning a clean error.Solution
runNotifyUpgradeEnow explicitly performs the setup steps it needs, usingcmd.Root()for flag/env/config binding. Temp-file creation targets the working directory, container ID detection falls back to filesystem-based methods that do not require a Docker client, and a newBuildURLshelper returns errors for unknown legacy notification types instead of exiting the process.Changes
notify-upgradeviacmd.Root().PersistentFlags()andappConfig.Load(cmd.Root(), nil)flags.SetupLoggingso--log-format,--log-level, and--no-colorare honorednotifications.NewNotifierwith the newnotifications.BuildURLsto avoidlogrus.Fatalon invalid legacy notification configcurrentWatchtowerContainerIDcheck with local filesystem-based detection usingcontainer.GetContainerIDFromMountinfoandcontainer.GetContainerIDFromCgroupFileos.CreateTemp(".", ...)docs/notifications/deprecations/migration-tool/index.mdwith usage guidance, per-service walkthroughs, file retrieval steps, and an automated migration scriptSummary by CodeRabbit
New Features
notify-upgrademigration to convert legacy notification settings into Shoutrrr URLs.Bug Fixes
Documentation