No changes message for github custom template update to INF from ERR for better logging #6422
Conversation
WalkthroughUpdated error handling in Update (Git pull) in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Updater as Update()
participant Git as pullChanges()
participant Logger as Logger
Updater->>Git: call pullChanges()
alt success
Git-->>Updater: nil
Updater->>Logger: log Debug/Info (success)
else already up-to-date
Git-->>Updater: error (git.NoErrAlreadyUpToDate)
Updater->>Logger: log Info ("already up-to-date")
else other error
Git-->>Updater: wrapped error (errors.Wrapf(... "%s/%s"))
Updater->>Logger: log Error (propagate/handle)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/external/customtemplates/github.go (4)
56-60: Prefer sentinel error check over string matching for “already up-to-date”String matching on error messages is brittle. go-git exposes the sentinel
git.NoErrAlreadyUpToDate; useerrors.Isagainst it and log a clear, structured message.Apply this diff:
- if strings.Contains(err.Error(), "already up-to-date") { - gologger.Info().Msgf("%s", err) - } else { - gologger.Error().Msgf("%s", err) - } + if errors.Is(err, git.NoErrAlreadyUpToDate) { + gologger.Info().Msgf("Repo %s/%s is already up-to-date.", customTemplate.owner, customTemplate.reponame) + } else { + gologger.Error().Msgf("%s", err) + }Note: To make
errors.Iswork reliably here, ensurepullChangesreturns the sentinel (not just a string-wrapped error). See suggested change on Lines 192-195.
192-195: Propagate sentinel error from pullChanges to enable robust handling upstream
pullChangescurrently discards the original error by formatting it into a new string, making it impossible to distinguish “already up-to-date” at the call site without string matching. Return the sentinel directly for that case; for other errors, wrap (or annotate) while preserving the cause.Apply this diff:
err = w.Pull(pullOpts) if err != nil { - return errors.Errorf("%s/%s: %s", ctr.owner, ctr.reponame, err.Error()) + if errors.Is(err, git.NoErrAlreadyUpToDate) { + // propagate sentinel so callers can detect and downgrade to info + return git.NoErrAlreadyUpToDate + } + // keep context but avoid losing the original cause + return errors.Errorf("%s/%s: pull failed: %s", ctr.owner, ctr.reponame, err.Error()) }Optional follow-up: if the codebase allows, prefer wrapping the cause using
%w(fmt.Errorf) orerrors.Wrapfto preserve the chain forerrors.Is/As.
62-62: Remove trailing newline in log message for consistency
gologgeradds line endings; the explicit “\n” is unnecessary and can cause formatting inconsistencies.Apply this diff:
- gologger.Info().Msgf("Repo %s/%s successfully pulled the changes.\n", customTemplate.owner, customTemplate.reponame) + gologger.Info().Msgf("Repo %s/%s successfully pulled the changes.", customTemplate.owner, customTemplate.reponame)
49-51: Typo in comment: “exits” → “exists”Minor doc polish.
Apply this diff:
- // If folder does not exits then clone/download the repo + // If folder does not exist then clone/download the repo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/external/customtemplates/github.go(1 hunks)
| @@ -53,7 +53,11 @@ func (customTemplate *customTemplateGitHubRepo) Update(ctx context.Context) { | |||
| } | |||
| err := customTemplate.pullChanges(clonePath, customTemplate.githubToken) | |||
| if err != nil { | |||
| gologger.Error().Msgf("%s", err) | |||
| if strings.Contains(err.Error(), "already up-to-date") { | |||
There was a problem hiding this comment.
| if strings.Contains(err.Error(), "already up-to-date") { | |
| if errors.Is(err, git.NoErrAlreadyUpToDate) { |
There was a problem hiding this comment.
This doesn't appears to be working as if errors.Is(err, git.NoErrAlreadyUpToDate) is returning false
So, I've adjusted Errorf to Wrapf on PullChanges ( w.Pull ) and now it's working, please check...
Co-authored-by: Dwi Siswanto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/external/customtemplates/github.go (3)
194-194: Prefer stdlib wrapping with %w to guarantee errors.Is/As interopWrapping with
pkg/errors.Wrapfcan be fine, but using the stdlibfmt.Errorf(... %w ...)avoids any ambiguity and guarantees seamless behavior witherrors.Is/Asacross toolchains. It also ensures the upstream sentinel is preserved without relying on third-party unwrapping semantics.Apply this diff in-place:
- return errors.Wrapf(err, "%s/%s", ctr.owner, ctr.reponame) + return fmt.Errorf("%s/%s: %w", ctr.owner, ctr.reponame, err)Additional small change outside this hunk (imports):
// add alongside existing imports import "fmt"If you keep
pkg/errors, ensure it’s a version that implementsUnwrap()soerrors.Iskeeps working; otherwise the check in Update may misclassify the condition.
61-63: Remove redundant newline in Msgf
Msgfdoesn’t need a trailing newline; it’s handled by the logger. Minor polish.- gologger.Info().Msgf("Repo %s/%s successfully pulled the changes.\n", customTemplate.owner, customTemplate.reponame) + gologger.Info().Msgf("Repo %s/%s successfully pulled the changes.", customTemplate.owner, customTemplate.reponame)
56-60: Harden “already up-to-date” detection and clarify INFO messageNice catch demoting this to Info. To make the check robust against wrapped errors (and pkg/errors vs. stdlib interop), you can test both
errors.Isand a direct equality againstgit.NoErrAlreadyUpToDate, and log a more user-friendly message that includes the repo context:- if errors.Is(err, git.NoErrAlreadyUpToDate) { - gologger.Info().Msgf("%s", err) - } else { - gologger.Error().Msgf("%s", err) - } + // treat "already up-to-date" as an INFO (no-op) condition + if stderrors.Is(err, git.NoErrAlreadyUpToDate) || err == git.NoErrAlreadyUpToDate { + gologger.Info().Msgf("Repo %s/%s has no changes (already up-to-date).", customTemplate.owner, customTemplate.reponame) + } else { + gologger.Error().Msgf("%s", err) + }Don’t forget to alias the stdlib
errorspackage so it doesn’t clash withgithub.com/pkg/errors:import ( stderrors "errors" // …other imports… )This change will ensure the up-to-date case always logs at Info level and provides clearer context for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/external/customtemplates/github.go(2 hunks)
Proposed changes
No changes message for github custom template update can be
[INF]instead of[ERR]for better loggingChecklist
Summary by CodeRabbit