Post update hook - #134
Post update hook#134githubkusi wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a nullable post-update hook field, stores it in EF Core, loads it from Docker labels, runs configured scripts after successful updates, and documents the hook contract and environment variables. ChangesPost-update hook feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PatchPanda.Web/Services/DockerService.cs`:
- Line 18: Remove the unused HookService dependency from DockerService: delete
the private readonly field _hookService, remove the HookService parameter from
the DockerService constructor and its assignment, and update any constructor
call sites to stop passing a HookService; ensure no other references to
_hookService remain in DockerService (the hook execution belongs in
UpdateService), and run a build to verify compilation.
- Around line 20-27: The DockerService constructor now requires a HookService
parameter, so update the Mock<DockerService> usages in UpdateServiceTests to
supply a HookService mock: create a Mock<HookService> (e.g., hookServiceMock)
and pass hookServiceMock.Object as the final argument to each
Mock<DockerService> constructor call that currently constructs DockerService
with logger, IDbContextFactory<DataContext>, IVersionService, IPortainerService,
and IFileService; ensure the symbol names match (DockerService constructor and
HookService) and the new mock is used where the other service mocks are
provided.
In `@PatchPanda.Web/Services/HookServices.cs`:
- Around line 59-62: The hook runner (ExecuteHookAsync) currently only logs
non-zero exit codes, so callers like UpdateService can't react via their
existing exception handling; change the non-zero path in ExecuteHookAsync to
throw an exception (e.g., InvalidOperationException or a specific
HookExecutionException) that includes the scriptPath and process.ExitCode in the
message, while optionally keeping the _logger.LogError call for diagnostics, so
UpdateService's catch block will be triggered on hook failures.
- Around line 44-52: Ensure the process output streams are started reading
immediately after process.Start() and before awaiting process termination: call
process.StandardOutput.ReadToEndAsync() and
process.StandardError.ReadToEndAsync() right after process.Start(), store the
resulting stdoutTask and stderrTask, then await process.WaitForExitAsync() and
finally await stdoutTask/stderrTask to collect outputs; reference the Start(),
StandardOutput.ReadToEndAsync(), StandardError.ReadToEndAsync(),
WaitForExitAsync(), stdoutTask and stderrTask symbols when making the change.
- Around line 27-34: The ProcessStartInfo code interpolates scriptPath into a
bash command (FileName="/bin/bash", Arguments=$"-c \"{scriptPath}\"") which
enables command injection and breaks on Windows; change it to avoid shell
interpolation by launching the script directly (use ProcessStartInfo.FileName =
scriptPath or use ProcessStartInfo.ArgumentList to pass the script as an
argument) and add OS branching using RuntimeInformation.IsOSPlatform to choose a
shell only when necessary (e.g., use /bin/bash on Linux when running shell
commands, use pwsh or cmd on Windows), and validate/normalize scriptPath (ensure
it is an absolute path, exists, and contains no unsafe characters) before
starting the process; update the code in HookServices.cs around the
ProcessStartInfo creation (references: ProcessStartInfo, FileName, Arguments,
scriptPath) accordingly.
In `@PatchPanda.Web/Services/UpdateService.cs`:
- Around line 778-803: The current post-update hook invocation uses
stack.ConfigFile (possibly null) as the PP_PROJECT_DIR arg in
_hookService.ExecuteHookAsync, which causes hooks that do "cd $PP_PROJECT_DIR"
to fail for Portainer-managed stacks; update the block that checks
targetApp.PostUpdateHook to first detect stack.ConfigFile == null and in that
case either (preferred) skip executing the hook and call _logger.LogWarning with
context (targetApp.Name and that it's a Portainer-managed stack), or
(alternative) supply a safe fallback path (e.g., a dedicated runtime directory)
instead of string.Empty; ensure the chosen behavior is applied where
_hookService.ExecuteHookAsync is invoked so hooks never receive an empty project
dir.
In `@README.md`:
- Around line 233-244: The README example post-update hook is missing
surrounding blank lines for the heading and uses unquoted shell variables which
break on paths with spaces; update the script under the "Example post-update
hook script" to add a blank line before and after the fenced bash block, and
quote variables by changing uses of PP_PROJECT_DIR to "$PP_PROJECT_DIR" and
interpolate version variables in the commit message as "${PP_OLD_VERSION}" and
"${PP_NEW_VERSION}" so the git commit -m line reads a safe, properly expanded
string.
- Around line 214-223: The "Post-hooks" section in README.md needs proper
Markdown spacing for linting: add a blank line above and below the "##
Post-hooks" heading and ensure there is a blank line before the fenced code
block and after its closing ```; also ensure the fenced block specifies the
language (```yaml) and the code block content remains unchanged — update the
block around the "myservice" labels example so the heading and code fence have
blank lines on both sides to satisfy the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d97f459-2cc0-4d19-afcb-6957c1049dfe
📒 Files selected for processing (9)
PatchPanda.Web/Entities/Container.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.Designer.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.csPatchPanda.Web/Migrations/DataContextModelSnapshot.csPatchPanda.Web/Program.csPatchPanda.Web/Services/DockerService.csPatchPanda.Web/Services/HookServices.csPatchPanda.Web/Services/UpdateService.csREADME.md
|
It would be great to get a pre-update hook as well. My use case is to create a zfs snapshot of my data directory before an update to make rollbacks trivial. |
|
Hey @githubkusi, thank you for the PR! Could you please address coderabbit's comments before it's merged? And for the pre-update @benssson, since we already have most of the logic here once this is merged then should be easy to implement. |
|
Hey @dkorecko, yes, sure, I'll address coderabbits comments, I just wanted to make sure you're happy with the general direction of this PR before I invest time. |
The direction looks good to me! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
PatchPanda.Web/Services/HookServices.cs (1)
64-71: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo timeout on hook process execution.
If the hook script hangs (e.g., waiting for input, network stall, infinite loop),
WaitForExitAsyncblocks indefinitely. This stalls the update background service, leaves the job in "processing" state, and prevents future updates. Consider adding a configurable timeout with process termination on expiry.🔒 Proposed fix: add timeout with process kill
using var process = new Process { StartInfo = startInfo }; process.Start(); var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); -await process.WaitForExitAsync(); +var timeout = TimeSpan.FromMinutes(5); +if (!await process.WaitForExitAsync(timeout)) +{ + process.Kill(entireProcessTree: true); + throw new TimeoutException($"Hook script '{scriptPath}' timed out after {timeout.TotalMinutes} minutes."); +} var stdout = await stdoutTask; var stderr = await stderrTask;🤖 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 `@PatchPanda.Web/Services/HookServices.cs` around lines 64 - 71, The hook execution in HookServices’s process flow can block forever because WaitForExitAsync has no timeout. Add a configurable timeout around the process execution in the same method that starts the Process and reads stdout/stderr, then terminate the hook process if it exceeds the limit and handle that failure path cleanly so the background update service can continue.PatchPanda.Web/Services/DockerService.cs (1)
170-175: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
PostUpdateHookis not synced for existing containers inResetComposeStacks.The new
PostUpdateHookfield is populated when creating a newContainer(line 170-175), but when an existing container is updated during reset (lines 289-300),PostUpdateHookis not copied from the running container to the existing database record. If a user adds, changes, or removes thepatchpanda.hook.post_updateDocker label, the stale value persists in the database after a reset.🐛 Proposed fix: sync PostUpdateHook for existing containers
existingContainer.Uptime = runningContainer.Uptime; existingContainer.CurrentSha = runningContainer.CurrentSha; existingContainer.GitHubRepo = runningContainer.GitHubRepo; existingContainer.SecondaryGitHubRepos = runningContainer.SecondaryGitHubRepos; existingContainer.Version = runningContainer.Version; existingContainer.TargetImage = runningContainer.TargetImage; existingContainer.Regex = runningContainer.Regex; + existingContainer.PostUpdateHook = runningContainer.PostUpdateHook;🤖 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 `@PatchPanda.Web/Services/DockerService.cs` around lines 170 - 175, The `PostUpdateHook` value is being set when creating a new container in `DockerService`, but `ResetComposeStacks` does not copy that field onto an existing container record during updates, leaving stale data in the database. Update the existing-container sync path in `ResetComposeStacks` to read the current `patchpanda.hook.post_update` label and assign it to `PostUpdateHook` on the persisted container, matching the logic used in the container creation flow.PatchPanda.Web/Services/UpdateService.cs (1)
2-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove
PatchPanda.ServicestoGlobalUsings.cs
UpdateService.csshould not declare this namespace locally; addglobal using PatchPanda.Services;toPatchPanda.Web/GlobalUsings.csand remove the file-level import here.🤖 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 `@PatchPanda.Web/Services/UpdateService.cs` at line 2, The file-level `using PatchPanda.Services;` in `UpdateService.cs` should be removed and replaced with a `global using PatchPanda.Services;` in `GlobalUsings.cs` so the namespace is shared across the project. Update the `UpdateService` file to rely on the global import and keep the namespace declaration centralized in `GlobalUsings.cs`.Source: Coding guidelines
🤖 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 `@PatchPanda.Web/Services/HookServices.cs`:
- Line 42: Remove the explanatory inline comment in HookServices, since the code
should be self-documenting instead of relying on narration. Update the relevant
logic around the script-path argument handling in the HookServices code path,
and keep the implementation clear enough that the comment is no longer needed.
---
Outside diff comments:
In `@PatchPanda.Web/Services/DockerService.cs`:
- Around line 170-175: The `PostUpdateHook` value is being set when creating a
new container in `DockerService`, but `ResetComposeStacks` does not copy that
field onto an existing container record during updates, leaving stale data in
the database. Update the existing-container sync path in `ResetComposeStacks` to
read the current `patchpanda.hook.post_update` label and assign it to
`PostUpdateHook` on the persisted container, matching the logic used in the
container creation flow.
In `@PatchPanda.Web/Services/HookServices.cs`:
- Around line 64-71: The hook execution in HookServices’s process flow can block
forever because WaitForExitAsync has no timeout. Add a configurable timeout
around the process execution in the same method that starts the Process and
reads stdout/stderr, then terminate the hook process if it exceeds the limit and
handle that failure path cleanly so the background update service can continue.
In `@PatchPanda.Web/Services/UpdateService.cs`:
- Line 2: The file-level `using PatchPanda.Services;` in `UpdateService.cs`
should be removed and replaced with a `global using PatchPanda.Services;` in
`GlobalUsings.cs` so the namespace is shared across the project. Update the
`UpdateService` file to rely on the global import and keep the namespace
declaration centralized in `GlobalUsings.cs`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9739ec2-fef1-414d-ac85-88720c6d5e26
📒 Files selected for processing (4)
PatchPanda.Web/Services/DockerService.csPatchPanda.Web/Services/HookServices.csPatchPanda.Web/Services/UpdateService.csREADME.md
d930b85 to
117428e
Compare
Hook service after an update has been performed
Author: Copilot
117428e to
d65e15c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PatchPanda.Web/Services/DockerService.cs (1)
295-315: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync
PostUpdateHookfrom running containers to existing containers.
PostUpdateHookis loaded from the Docker label (lines 176-181) but is missing from the existing-container update block. This means the hook only works for containers discovered for the first time — any container already in the database will never pick up a new or changedpatchpanda.hook.post_updatelabel.🐛 Proposed fix: add PostUpdateHook to the sync block
if (existingContainer is not null) { existingContainer.Uptime = runningContainer.Uptime; existingContainer.CurrentSha = runningContainer.CurrentSha; existingContainer.GitHubRepo = runningContainer.GitHubRepo; existingContainer.SecondaryGitHubRepos = runningContainer.SecondaryGitHubRepos; existingContainer.Version = runningContainer.Version; existingContainer.TargetImage = runningContainer.TargetImage; existingContainer.Regex = runningContainer.Regex; + existingContainer.PostUpdateHook = runningContainer.PostUpdateHook; if (🤖 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 `@PatchPanda.Web/Services/DockerService.cs` around lines 295 - 315, The existing-container merge in DockerService should also sync the PostUpdateHook value from runningContainer to existingContainer, since it is already populated from the Docker label when containers are discovered. Update the sync block in the existingContainer branch to copy PostUpdateHook alongside the other fields, using the same pattern as GitHubRepo, Version, and TargetImage so database-backed containers pick up hook changes.
🤖 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 `@PatchPanda.Web/Program.cs`:
- Line 3: Move the PatchPanda.Services import out of Program and into
GlobalUsings, since the coding guideline is to keep shared namespace imports
centralized there. Update Program.cs to remove the per-file using and add the
same import to PatchPanda.Web/GlobalUsings.cs so any references to symbols from
PatchPanda.Services continue to resolve through the shared global using.
In `@PatchPanda.Web/Services/DockerService.cs`:
- Line 247: The DbContext disposal in the async flow should use asynchronous
disposal instead of synchronous disposal. Update the database access in the
DockerService method that creates the context with
_dbContextFactory.CreateDbContext() to use await using so the DbContext is
disposed correctly in the async method.
In `@PatchPanda.Web/Services/HookServices.cs`:
- Line 1: Move the System.Diagnostics import out of HookServices and into
GlobalUsings so the namespace is available project-wide. Remove the per-file
using statement from HookServices and add the equivalent global using in
GlobalUsings.cs, following the existing global import pattern used by the
project.
In `@PatchPanda.Web/Services/UpdateService.cs`:
- Line 2: Move the PatchPanda.Services namespace import out of UpdateService and
into PatchPanda.Web/GlobalUsings, since this project expects shared imports to
live in the global usings file instead of per-file using statements. Remove the
using from UpdateService and add the corresponding global import in GlobalUsings
so any types referenced there still resolve through the shared namespace setup.
---
Outside diff comments:
In `@PatchPanda.Web/Services/DockerService.cs`:
- Around line 295-315: The existing-container merge in DockerService should also
sync the PostUpdateHook value from runningContainer to existingContainer, since
it is already populated from the Docker label when containers are discovered.
Update the sync block in the existingContainer branch to copy PostUpdateHook
alongside the other fields, using the same pattern as GitHubRepo, Version, and
TargetImage so database-backed containers pick up hook changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed152f4a-95fd-455d-8ebc-04b9704bfe7b
📒 Files selected for processing (9)
PatchPanda.Web/Entities/Container.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.Designer.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.csPatchPanda.Web/Migrations/DataContextModelSnapshot.csPatchPanda.Web/Program.csPatchPanda.Web/Services/DockerService.csPatchPanda.Web/Services/HookServices.csPatchPanda.Web/Services/UpdateService.csREADME.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PatchPanda.Web/Services/DockerService.cs (1)
295-315: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync
PostUpdateHookfrom running containers to existing containers.
PostUpdateHookis loaded from the Docker label (lines 176-181) but is missing from the existing-container update block. This means the hook only works for containers discovered for the first time — any container already in the database will never pick up a new or changedpatchpanda.hook.post_updatelabel.🐛 Proposed fix: add PostUpdateHook to the sync block
if (existingContainer is not null) { existingContainer.Uptime = runningContainer.Uptime; existingContainer.CurrentSha = runningContainer.CurrentSha; existingContainer.GitHubRepo = runningContainer.GitHubRepo; existingContainer.SecondaryGitHubRepos = runningContainer.SecondaryGitHubRepos; existingContainer.Version = runningContainer.Version; existingContainer.TargetImage = runningContainer.TargetImage; existingContainer.Regex = runningContainer.Regex; + existingContainer.PostUpdateHook = runningContainer.PostUpdateHook; if (🤖 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 `@PatchPanda.Web/Services/DockerService.cs` around lines 295 - 315, The existing-container merge in DockerService should also sync the PostUpdateHook value from runningContainer to existingContainer, since it is already populated from the Docker label when containers are discovered. Update the sync block in the existingContainer branch to copy PostUpdateHook alongside the other fields, using the same pattern as GitHubRepo, Version, and TargetImage so database-backed containers pick up hook changes.
🤖 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 `@PatchPanda.Web/Program.cs`:
- Line 3: Move the PatchPanda.Services import out of Program and into
GlobalUsings, since the coding guideline is to keep shared namespace imports
centralized there. Update Program.cs to remove the per-file using and add the
same import to PatchPanda.Web/GlobalUsings.cs so any references to symbols from
PatchPanda.Services continue to resolve through the shared global using.
In `@PatchPanda.Web/Services/DockerService.cs`:
- Line 247: The DbContext disposal in the async flow should use asynchronous
disposal instead of synchronous disposal. Update the database access in the
DockerService method that creates the context with
_dbContextFactory.CreateDbContext() to use await using so the DbContext is
disposed correctly in the async method.
In `@PatchPanda.Web/Services/HookServices.cs`:
- Line 1: Move the System.Diagnostics import out of HookServices and into
GlobalUsings so the namespace is available project-wide. Remove the per-file
using statement from HookServices and add the equivalent global using in
GlobalUsings.cs, following the existing global import pattern used by the
project.
In `@PatchPanda.Web/Services/UpdateService.cs`:
- Line 2: Move the PatchPanda.Services namespace import out of UpdateService and
into PatchPanda.Web/GlobalUsings, since this project expects shared imports to
live in the global usings file instead of per-file using statements. Remove the
using from UpdateService and add the corresponding global import in GlobalUsings
so any types referenced there still resolve through the shared namespace setup.
---
Outside diff comments:
In `@PatchPanda.Web/Services/DockerService.cs`:
- Around line 295-315: The existing-container merge in DockerService should also
sync the PostUpdateHook value from runningContainer to existingContainer, since
it is already populated from the Docker label when containers are discovered.
Update the sync block in the existingContainer branch to copy PostUpdateHook
alongside the other fields, using the same pattern as GitHubRepo, Version, and
TargetImage so database-backed containers pick up hook changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed152f4a-95fd-455d-8ebc-04b9704bfe7b
📒 Files selected for processing (9)
PatchPanda.Web/Entities/Container.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.Designer.csPatchPanda.Web/Migrations/20260323223243_AddPostUpdateHook.csPatchPanda.Web/Migrations/DataContextModelSnapshot.csPatchPanda.Web/Program.csPatchPanda.Web/Services/DockerService.csPatchPanda.Web/Services/HookServices.csPatchPanda.Web/Services/UpdateService.csREADME.md
🛑 Comments failed to post (4)
PatchPanda.Web/Program.cs (1)
3-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move
using PatchPanda.Services;toGlobalUsings.cs.As per coding guidelines, namespace imports should be added to
PatchPanda.Web/GlobalUsings.csinstead of per-fileusingstatements.🤖 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 `@PatchPanda.Web/Program.cs` at line 3, Move the PatchPanda.Services import out of Program and into GlobalUsings, since the coding guideline is to keep shared namespace imports centralized there. Update Program.cs to remove the per-file using and add the same import to PatchPanda.Web/GlobalUsings.cs so any references to symbols from PatchPanda.Services continue to resolve through the shared global using.Source: Coding guidelines
PatchPanda.Web/Services/DockerService.cs (1)
247-247: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use
await usingforDbContextin async method.As per coding guidelines, when accessing the database in async methods, use
await using var db = _dbContextFactory.CreateDbContext()instead ofusing.♻️ Proposed fix
- using var db = _dbContextFactory.CreateDbContext(); + await using var db = _dbContextFactory.CreateDbContext();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await using var db = _dbContextFactory.CreateDbContext();🤖 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 `@PatchPanda.Web/Services/DockerService.cs` at line 247, The DbContext disposal in the async flow should use asynchronous disposal instead of synchronous disposal. Update the database access in the DockerService method that creates the context with _dbContextFactory.CreateDbContext() to use await using so the DbContext is disposed correctly in the async method.Source: Coding guidelines
PatchPanda.Web/Services/HookServices.cs (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move
using System.Diagnostics;toGlobalUsings.cs.As per coding guidelines, namespace imports should be added to
PatchPanda.Web/GlobalUsings.csinstead of per-fileusingstatements.🤖 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 `@PatchPanda.Web/Services/HookServices.cs` at line 1, Move the System.Diagnostics import out of HookServices and into GlobalUsings so the namespace is available project-wide. Remove the per-file using statement from HookServices and add the equivalent global using in GlobalUsings.cs, following the existing global import pattern used by the project.Source: Coding guidelines
PatchPanda.Web/Services/UpdateService.cs (1)
2-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move
using PatchPanda.Services;toGlobalUsings.cs.Per the project's coding guidelines, namespace imports should be added to
PatchPanda.Web/GlobalUsings.csinstead of per-fileusingstatements. This new import should be placed there rather than in this source file.As per coding guidelines: "Add namespace imports to
PatchPanda.Web/GlobalUsings.csinstead of per-fileusingstatements."♻️ Proposed change
Remove from
UpdateService.cs:-using PatchPanda.Services;Add to
PatchPanda.Web/GlobalUsings.cs:+global using PatchPanda.Services;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🤖 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 `@PatchPanda.Web/Services/UpdateService.cs` at line 2, Move the PatchPanda.Services namespace import out of UpdateService and into PatchPanda.Web/GlobalUsings, since this project expects shared imports to live in the global usings file instead of per-file using statements. Remove the using from UpdateService and add the corresponding global import in GlobalUsings so any types referenced there still resolve through the shared namespace setup.Source: Coding guidelines
|
I believe I've addressed all coderabbit complaints. Example script from the readme works fine for me. Looks good to you @dkorecko ? |
Only one comment, but looks good other than that! Once that's addressed and if workflows pass, we'll be good to merge and get this released 👍 |
|
Also, the app does not build as you can see 😄 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
PatchPanda.Web/Services/UpdateService.cs (2)
820-826: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound post-update hook execution.
ExecuteHookAsyncstarts a child process and waits without a cancellation token or timeout. A hanging hook can leaveUpdateawaiting indefinitely after the database has recorded the update as successful. Add bounded cancellation/timeout handling inHookServiceand pass the update token here.🤖 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 `@PatchPanda.Web/Services/UpdateService.cs` around lines 820 - 826, Update HookService.ExecuteHookAsync to accept and enforce a cancellation token with a bounded timeout while awaiting the child process, then pass the existing update cancellation token from the Update flow at the shown call site. Preserve successful hook execution while ensuring a hanging or cancelled post-update hook cannot leave Update awaiting indefinitely.
2-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove this namespace import to
PatchPanda.Web/GlobalUsings.cs.
UpdateService.csshould not add a per-file namespace import forHookService; centralize it in the Web project’s global usings file.As per coding guidelines,
**/*.csimports must be added toPatchPanda.Web/GlobalUsings.csinstead of per-fileusingstatements.🤖 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 `@PatchPanda.Web/Services/UpdateService.cs` at line 2, Remove the PatchPanda.Services using directive from UpdateService.cs and add the corresponding global using to PatchPanda.Web/GlobalUsings.cs, keeping the file’s HookService references compiling through the centralized import.Source: Coding guidelines
🤖 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 `@PatchPanda.Units/Services/UpdateServiceTests.cs`:
- Around line 53-55: Add behavioral tests around the test factory’s HookService
wiring, covering successful PostUpdateHook execution, hook failure handling, and
the Portainer skip branch. Prefer injecting a deterministic hook abstraction
into the relevant UpdateService tests, and assert each path’s observable
behavior without changing unrelated service setup.
---
Outside diff comments:
In `@PatchPanda.Web/Services/UpdateService.cs`:
- Around line 820-826: Update HookService.ExecuteHookAsync to accept and enforce
a cancellation token with a bounded timeout while awaiting the child process,
then pass the existing update cancellation token from the Update flow at the
shown call site. Preserve successful hook execution while ensuring a hanging or
cancelled post-update hook cannot leave Update awaiting indefinitely.
- Line 2: Remove the PatchPanda.Services using directive from UpdateService.cs
and add the corresponding global using to PatchPanda.Web/GlobalUsings.cs,
keeping the file’s HookService references compiling through the centralized
import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23476420-1f3e-49d3-bbb7-a295269b5560
📒 Files selected for processing (3)
PatchPanda.Units/GlobalUsings.csPatchPanda.Units/Services/UpdateServiceTests.csPatchPanda.Web/Services/UpdateService.cs
| _notificationService.Object, | ||
| new HookService(new Mock<ILogger<HookService>>().Object) | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Add behavioral coverage for post-update hooks.
The test factory now wires HookService, but no test configures PostUpdateHook or verifies execution, failure handling, or the Portainer skip branch. Add deterministic tests for these paths, preferably through an injectable hook abstraction.
🤖 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 `@PatchPanda.Units/Services/UpdateServiceTests.cs` around lines 53 - 55, Add
behavioral tests around the test factory’s HookService wiring, covering
successful PostUpdateHook execution, hook failure handling, and the Portainer
skip branch. Prefer injecting a deterministic hook abstraction into the relevant
UpdateService tests, and assert each path’s observable behavior without changing
unrelated service setup.
|
Now, I guess coderabbit complains about not having tests for the post-update hook functionality. You want tests for theses as well? |
|
@githubkusi looks good, thanks! So you've been using this for some time now with no issues? Would you say it's good to be released? |
|
@dkorecko I just tested it locally so far, let me also run it for a while on my productive system. I will let you know |
Previously ResetComposeStacks only set PostUpdateHook when creating a new Container entity, so changing the patchpanda.hook.post_update label on an already-tracked container was never picked up on subsequent rescans. Now it's synced like the other container fields.
|
I'm using it on my production system now, all fine. I added a small fix which avoids the need to restart PatchPanda after a label change. And updated the readme. Other than that it's ready to go I guess @dkorecko |
3 participants
This PR implements a post update hook, which allows to run a script after a successful update. The script is run within the PatchPanda container, but has access to the host filesystem. Sample script:
A typical use case includes an automated git commit of the docker-compose.yml. Unfortunately, the current image mcr.microsoft.com/dotnet/aspnet doesn't include git, hence it need to be installed from within the script. In order to simplify post update scripts for the git use-case, the installation of git could be done within the Dockerfile. What do you think?
This PR was created with significant help of Github Copilot
fixes #89
Summary by CodeRabbit
New Features
patchpanda.hook.post_updateand stored per container; if the hook or compose file is missing/blank, the hook is skipped.PP_PROJECT_DIR,PP_NAME,PP_OLD_VERSION,PP_NEW_VERSION, andPP_UPDATE_TIME. Any hook execution errors are logged without failing the update.Documentation