refactor(log): replace WithFields with format strings#736
Merged
Conversation
Replace all WithFields(logrus.Fields{...}).Level() calls with Levelf()
format-string equivalents across 44 files, removing the dependency on
logrus structured fields for application logging.
📝 WalkthroughWalkthroughThis PR replaces structured Logrus field-based logging with formatted logger calls across numerous files, removing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (1)
cmd/agent/workspace/install_dotfiles.go (1)
151-151: IncludescriptPathin failure debug log for retry-loop diagnostics.At Line 151, only the error is logged. In a multi-script loop, this loses which candidate failed.
Proposed tweak
- logger.Debugf("script execution failed: error=%v", err) + logger.Debugf("script execution failed: scriptPath=%s, error=%v", installScriptPath, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/workspace/install_dotfiles.go` at line 151, The debug log for failed script execution omits which script failed; update the logger.Debugf call in the retry loop (the place referencing logger and scriptPath) to include scriptPath along with the error (e.g., logger.Debugf("script execution failed: script=%s error=%v", scriptPath, err)) so retry-loop diagnostics can identify the failing candidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 706-711: The debug log call s.log.Debugf currently passes
arguments in the wrong order which labels buf.String() as stdout and
stdout.String() as stderr; update the call in workspace_client.go
(s.log.Debugf(...) near the container status command) to pass stdout.String() as
the first %s and buf.String() as the second %s so the "stdout=%s, stderr=%s,
parsed=%v" format matches the actual variables (ensure you keep parsed as the
third argument and that both buf and stdout implement String()).
In `@pkg/credentials/server.go`:
- Line 107: The code currently logs full POST bodies via log.Debugf (e.g. the
"received docker credentials post data" call and the similar Debugf calls at the
other occurrences), which may contain secrets; replace those raw-payload debug
logs with either (a) a sanitized log that redacts sensitive fields by parsing
the payload and removing keys like "password", "token", "signature", "auth",
"secret", or (b) a safe summary (e.g. payload size, JSON keys present, or a
hash) instead of the full body. Update each log.Debugf occurrence (the call that
prints string(out) and the similar calls at the other locations) to emit only
redacted/summarized information and ensure any structured logging uses
non-sensitive keys.
In `@pkg/devcontainer/feature/features.go`:
- Around line 44-45: The warning builds a raw string so the
`${escapeQuotesForShell(feature.id)}` placeholder is not interpolated; update
the construction of warningHeader (the variable containing the deprecated
message) to concatenate or format in the actual escaped feature id returned by
escapeQuotesForShell(feature.id) (e.g., use string concatenation or fmt.Sprintf)
instead of leaving the placeholder inside a raw back-quoted string literal so
the real feature id is injected into the message.
In `@pkg/devcontainer/setup/lifecyclehooks.go`:
- Around line 208-213: The log and error message currently hard-code
"postCreateCommand"; update them to use the current lifecycle hook variable
(e.g., hookName or similar used in this function) instead so failures reflect
the actual hook. Replace the "postCreateCommand" literal in the log.Debugf call
and in the returned fmt.Errorf with the hook variable, keeping cmd.Args and err
in the debug log and preserving the command string built from c in the error
return (format still: failed to run: <joined command>, error: %w) so the
messages show the correct hook name, command, and error.
---
Nitpick comments:
In `@cmd/agent/workspace/install_dotfiles.go`:
- Line 151: The debug log for failed script execution omits which script failed;
update the logger.Debugf call in the retry loop (the place referencing logger
and scriptPath) to include scriptPath along with the error (e.g.,
logger.Debugf("script execution failed: script=%s error=%v", scriptPath, err))
so retry-loop diagnostics can identify the failing candidate.
🪄 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: 6c42ff5c-e92b-47a9-871a-7cd3dcd3cf5a
📒 Files selected for processing (44)
cmd/agent/workspace/delete.gocmd/agent/workspace/install_dotfiles.gocmd/agent/workspace/up.gocmd/import.gocmd/pro/add/cluster.gocmd/pro/delete.gocmd/pro/import_workspace.gocmd/pro/login.gocmd/pro/sleep.gocmd/pro/start.gocmd/pro/wakeup.gocmd/provider/add.gocmd/provider/delete.gocmd/provider/set_options.gocmd/provider/update.gocmd/provider/use.gocmd/stop.gopkg/agent/agent.gopkg/agent/inject.gopkg/agent/tunnelserver/tunnelserver.gopkg/client/clientimplementation/machine_client.gopkg/client/clientimplementation/workspace_client.gopkg/credentials/server.gopkg/credentials/start.gopkg/devcontainer/build.gopkg/devcontainer/compose.gopkg/devcontainer/config/prebuild.gopkg/devcontainer/delete.gopkg/devcontainer/feature/features.gopkg/devcontainer/setup/lifecyclehooks.gopkg/devcontainer/setup/setup.gopkg/driver/docker/docker.gopkg/ide/jetbrains/generic.gopkg/ide/opener/opener.gopkg/ide/openvscode/openvscode.gopkg/ide/vscode/vscode.gopkg/inject/inject.gopkg/open/open.gopkg/ssh/ssh_add.gopkg/workspace/list.gopkg/workspace/machine.gopkg/workspace/pro.gopkg/workspace/provider_update.gopkg/workspace/workspace.go
Comment on lines
+44
to
+45
| warningHeader += `(!) WARNING: Using the deprecated Feature ` + | ||
| `"${escapeQuotesForShell(feature.id)}". This Feature will no longer receive any further updates/support.\n` |
There was a problem hiding this comment.
Deprecated-feature warning now prints a literal placeholder instead of the feature id
Line 45 is inside a raw string literal, so ${escapeQuotesForShell(feature.id)} is not evaluated and will be shown literally in the generated script.
🔧 Proposed fix
if feature.Deprecated {
- warningHeader += `(!) WARNING: Using the deprecated Feature ` +
- `"${escapeQuotesForShell(feature.id)}". This Feature will no longer receive any further updates/support.\n`
+ warningHeader += `(!) WARNING: Using the deprecated Feature "` +
+ id +
+ `". This Feature will no longer receive any further updates/support.\n`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/feature/features.go` around lines 44 - 45, The warning
builds a raw string so the `${escapeQuotesForShell(feature.id)}` placeholder is
not interpolated; update the construction of warningHeader (the variable
containing the deprecated message) to concatenate or format in the actual
escaped feature id returned by escapeQuotesForShell(feature.id) (e.g., use
string concatenation or fmt.Sprintf) instead of leaving the placeholder inside a
raw back-quoted string literal so the real feature id is injected into the
message.
Comment on lines
+208
to
213
| log.Debugf( | ||
| "failed running postCreateCommand lifecycle script: command=%v, error=%v", | ||
| cmd.Args, | ||
| err, | ||
| ) | ||
| return fmt.Errorf("failed to run: %s, error: %w", strings.Join(c, " "), err) |
There was a problem hiding this comment.
Use the current hook name in failure logs instead of a hard-coded one.
At Line 209, the message always says postCreateCommand, which is misleading when this path runs for other lifecycle hooks.
Suggested fix
- log.Debugf(
- "failed running postCreateCommand lifecycle script: command=%v, error=%v",
- cmd.Args,
- err,
- )
+ log.Debugf(
+ "failed running %s lifecycle script: command=%v, error=%v",
+ name,
+ cmd.Args,
+ err,
+ )📝 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.
Suggested change
| log.Debugf( | |
| "failed running postCreateCommand lifecycle script: command=%v, error=%v", | |
| cmd.Args, | |
| err, | |
| ) | |
| return fmt.Errorf("failed to run: %s, error: %w", strings.Join(c, " "), err) | |
| log.Debugf( | |
| "failed running %s lifecycle script: command=%v, error=%v", | |
| name, | |
| cmd.Args, | |
| err, | |
| ) | |
| return fmt.Errorf("failed to run: %s, error: %w", strings.Join(c, " "), err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/setup/lifecyclehooks.go` around lines 208 - 213, The log and
error message currently hard-code "postCreateCommand"; update them to use the
current lifecycle hook variable (e.g., hookName or similar used in this
function) instead so failures reflect the actual hook. Replace the
"postCreateCommand" literal in the log.Debugf call and in the returned
fmt.Errorf with the hook variable, keeping cmd.Args and err in the debug log and
preserving the command string built from c in the error return (format still:
failed to run: <joined command>, error: %w) so the messages show the correct
hook name, command, and error.
- Fix stdout/stderr argument swap in workspace_client.go debug log - Stop logging raw credential payloads in server.go, log byte length instead - Use dynamic hook name instead of hardcoded "postCreateCommand" in lifecyclehooks.go
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Replace all
WithFields(logrus.Fields{...}).Level()calls withLevelf()format-string equivalents across 44 filesSummary by CodeRabbit