Skip to content

actually log error on exit - #3344

Merged
butonic merged 1 commit into
mainfrom
antivirus-log-error
Aug 19, 2026
Merged

actually log error on exit#3344
butonic merged 1 commit into
mainfrom
antivirus-log-error

Conversation

@butonic

@butonic butonic commented Aug 18, 2026

Copy link
Copy Markdown
Member

exiting withoul logging is not nice ... fmt.Errorf ... does not log the error, it just renders the string and returns it ...

@codacy-production

codacy-production Bot commented Aug 18, 2026

Copy link
Copy Markdown

Up to standards βœ…

🟒 Issues 0 issues

Results:
0 new issues

View in Codacy

🟒 Metrics 31 duplication

Metric Results
Duplication 31

View in Codacy

🟒 Coverage 0.00% diff coverage · +0.00% coverage variation

Metric Results
Coverage variation βœ… +0.00% coverage variation (-1.00%)
Diff coverage βœ… 0.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (5056d4a) 84104 19349 23.01%
Head commit (dcd717f) 84104 (+0) 19349 (+0) 23.01% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3344) 1 0 0.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

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.

@pbleser-oc pbleser-oc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@butonic
butonic enabled auto-merge August 18, 2026 15:22
@dschmidt

Copy link
Copy Markdown
Contributor

Since #3140 you can simply return the error instead:

if err != nil {
    return fmt.Errorf("failed to initialize antivirus service: %w", err)
}

Supervised mode: the service runs in-process (runtime/service/service.go calls antivirus.Execute directly), so os.Exit(1) kills the whole opencloud process and bypasses suture's restart logic. Returning the error lets the supervisor log it structured via Err().

Standalone mode: since #3140 cobra is silenced, main() prints the error to stderr and exits 1, so nothing is lost there either.

Also matches the debug server block below, which already returns its error. The Str("server", "antivirus") field would be redundant anyway, the logger already carries the service name.

@dschmidt
dschmidt disabled auto-merge August 18, 2026 15:50
@dschmidt

dschmidt commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Sorry for disabling auto-merge, I would like you to look at my comment before it's merged πŸ™ˆ

If you disagree, just reenable auto-merge/click merge

edit:
The proper way would have probably been to request a change, but then it's more annoying to overrule than just reenabling auto-merge

@butonic
butonic merged commit 6d2cbd3 into main Aug 19, 2026
63 of 64 checks passed
@github-project-automation github-project-automation Bot moved this from Qualification to Done in OpenCloud Team Board Aug 19, 2026
@butonic
butonic deleted the antivirus-log-error branch August 19, 2026 07:39
@butonic

butonic commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@dschmidt Absolutely true. However, this is not the only place where we exit(1) 😞 I'll create an issue to track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants