Merged
Conversation
WalkthroughUpdated Go/tooling and container base images, added a Docker host configuration section to the README, and replaced the logging implementation and API across the codebase (new SlogLogger, changed Logger interface and Context logging), with corresponding test updates and minor Docker image tag bump. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
14-24:⚠️ Potential issue | 🟠 MajorRun the final image as non-root user.
The runtime stage has no
USER, soofeliaruns as root. That is a security hardening gap and is also flagged by static analysis.🔒 Suggested hardening patch
FROM alpine:3.23 # this label is required to identify container with ofelia running LABEL ofelia.service=true LABEL ofelia.enabled=true -RUN apk --no-cache add ca-certificates tini tzdata +RUN apk --no-cache add ca-certificates tini tzdata \ + && addgroup -S ofelia \ + && adduser -S -G ofelia ofelia COPY --from=builder /go/bin/ofelia /usr/bin/ofelia +USER ofelia ENTRYPOINT ["/sbin/tini", "/usr/bin/ofelia"]If you mount
/var/run/docker.sock, ensure runtime group permissions are handled (for example viagroup_addmatching socket GID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 14 - 24, The image launches ofelia as root; add a non-root runtime user and set USER before ENTRYPOINT so ofelia runs unprivileged—create a minimal user/group (e.g. ofelia user), ensure binary ownership and any required runtime dirs are chowned to that user (refer to the copied binary /usr/bin/ofelia and ENTRYPOINT), and document/handle socket access by allowing group membership or advising group_add for the Docker socket GID when mounting /var/run/docker.sock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Around line 14-24: The image launches ofelia as root; add a non-root runtime
user and set USER before ENTRYPOINT so ofelia runs unprivileged—create a minimal
user/group (e.g. ofelia user), ensure binary ownership and any required runtime
dirs are chowned to that user (refer to the copied binary /usr/bin/ofelia and
ENTRYPOINT), and document/handle socket access by allowing group membership or
advising group_add for the Docker socket GID when mounting /var/run/docker.sock.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fc9fff3-224f-4b7b-9e21-248acb876272
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
DockerfileREADME.mdgo.modintegration/test-run-exec/docker-compose.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cli/docker_handler.go (1)
118-118: Keep the original error in fallback log context.Line 118 logs the fallback action but omits
err, which makes root-cause diagnosis harder.🛠️ Suggested tweak
- c.logger.Debug("Failed to extract ofelia's container ID. Trying with container hostname instead...") + c.logger.Debug("Failed to extract ofelia's container ID. Trying with container hostname instead...", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/docker_handler.go` at line 118, The fallback debug log "Failed to extract ofelia's container ID. Trying with container hostname instead..." omits the original error; update the logging call in the Docker handler (the method in cli/docker_handler.go where c.logger.Debug is invoked) to include the original error variable (err) in the log context/message so the fallback log contains the root-cause error information (e.g., include err as a field or append err.Error() to the message).middlewares/common_test.go (1)
67-70: Consider aligning parameter names and types with the Logger interface.The
TestLoggermethods use(format string, args ...interface{})but theLoggerinterface incore/common.go:226-231defines(str string, args ...any). Whileinterface{}andanyare type aliases and this compiles correctly, matching the interface signature exactly improves consistency.♻️ Suggested alignment with interface
-func (*TestLogger) Debug(format string, args ...interface{}) {} -func (*TestLogger) Error(format string, args ...interface{}) {} -func (*TestLogger) Info(format string, args ...interface{}) {} -func (*TestLogger) Warning(format string, args ...interface{}) {} +func (*TestLogger) Debug(str string, args ...any) {} +func (*TestLogger) Error(str string, args ...any) {} +func (*TestLogger) Info(str string, args ...any) {} +func (*TestLogger) Warning(str string, args ...any) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middlewares/common_test.go` around lines 67 - 70, Update the TestLogger method signatures to exactly match the Logger interface by changing each method (Debug, Error, Info, Warning) to accept parameters named "str string, args ...any" instead of "format string, args ...interface{}"; this aligns parameter names and uses the same type alias (any) as in core/common.go's Logger interface for consistency.cli/config_test.go (1)
22-25: Consider renamingformatparameter tomsgorstrfor clarity.The parameter is named
formatbut the new logging API uses structured key-value pairs rather than printf-style formatting. Renaming tomsgorstr(matching theLoggerinterface incore/common.go) would better reflect the actual usage pattern.♻️ Suggested rename
-func (*TestLogger) Debug(format string, args ...interface{}) {} -func (*TestLogger) Error(format string, args ...interface{}) {} -func (*TestLogger) Info(format string, args ...interface{}) {} -func (*TestLogger) Warning(format string, args ...interface{}) {} +func (*TestLogger) Debug(msg string, args ...interface{}) {} +func (*TestLogger) Error(msg string, args ...interface{}) {} +func (*TestLogger) Info(msg string, args ...interface{}) {} +func (*TestLogger) Warning(msg string, args ...interface{}) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/config_test.go` around lines 22 - 25, Rename the parameter named `format` to `msg` (or `str`) in the TestLogger methods to match the structured logging API and the `Logger` interface in core/common.go: update the method signatures for Debug, Error, Info, and Warning on type `*TestLogger` so they accept `msg string, args ...interface{}` (or `str string, ...`) instead of `format string, ...`, and adjust any internal references to this parameter accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/cron_utils.go`:
- Around line 12-18: The Info method signature uses variadic ...any which breaks
implementation of robfig/cron v3 Logger; update CronUtils.Info to use the same
variadic type as CronUtils.Error (i.e., ...interface{}) so both Info and Error
match the cron.Logger interface expected by cron.WithLogger; modify the function
signature for CronUtils.Info accordingly while keeping its body unchanged.
In `@middlewares/slack.go`:
- Around line 64-67: The logs are currently emitting the sensitive Slack webhook
(m.SlackWebhook) via ctx.Logger.Error in the error paths; remove the webhook
from the log payload or replace it with a redacted/masked value before logging
to avoid secret exposure. Update the calls in the error handling branches (the
ctx.Logger.Error usages that reference m.SlackWebhook and r.Status/r.StatusCode)
to omit the "webhook" key or substitute a safe string like "<redacted webhook>"
or a masked snippet, ensuring no raw webhook URL appears in logs.
- Around line 62-67: The HTTP response from http.PostForm in
middlewares/slack.go is never closed which leaks connections; update the block
where r, err := http.PostForm(m.SlackWebhook, values) is handled so that when
err == nil you immediately ensure the body is closed (e.g., defer r.Body.Close()
or read io.Copy(io.Discard, r.Body) and then r.Body.Close()) before any returns
or logging; modify the handler around the PostForm call (referencing variables
r, err, m.SlackWebhook and ctx.Logger) to always close r.Body in both the 200
and non-200 response paths.
---
Nitpick comments:
In `@cli/config_test.go`:
- Around line 22-25: Rename the parameter named `format` to `msg` (or `str`) in
the TestLogger methods to match the structured logging API and the `Logger`
interface in core/common.go: update the method signatures for Debug, Error,
Info, and Warning on type `*TestLogger` so they accept `msg string, args
...interface{}` (or `str string, ...`) instead of `format string, ...`, and
adjust any internal references to this parameter accordingly.
In `@cli/docker_handler.go`:
- Line 118: The fallback debug log "Failed to extract ofelia's container ID.
Trying with container hostname instead..." omits the original error; update the
logging call in the Docker handler (the method in cli/docker_handler.go where
c.logger.Debug is invoked) to include the original error variable (err) in the
log context/message so the fallback log contains the root-cause error
information (e.g., include err as a field or append err.Error() to the message).
In `@middlewares/common_test.go`:
- Around line 67-70: Update the TestLogger method signatures to exactly match
the Logger interface by changing each method (Debug, Error, Info, Warning) to
accept parameters named "str string, args ...any" instead of "format string,
args ...interface{}"; this aligns parameter names and uses the same type alias
(any) as in core/common.go's Logger interface for consistency.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d96bfc98-709f-4838-b39d-236e71ee3781
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
README.mdcli/config.gocli/config_test.gocli/daemon.gocli/docker_handler.gocli/validate.gocore/common.gocore/common_test.gocore/cron_utils.gocore/logger.gocore/runjob.gocore/runjob_test.gocore/runservice.gocore/runservice_test.gocore/scheduler.gogo.modmiddlewares/common_test.gomiddlewares/mail.gomiddlewares/save.gomiddlewares/slack.goofelia.go
✅ Files skipped from review due to trivial changes (3)
- middlewares/save.go
- README.md
- go.mod
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
3 participants
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.
Putting it in Draft Mode due to upstream build failure
Summary by CodeRabbit
Documentation
Chores
Refactor