fix: Use log library #3
Conversation
WalkthroughThe changes in this pull request introduce enhanced logging functionality to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Outside diff range and nitpick comments (5)
cmd/ctrlc/root/api/create/relationship/jobtoresource/job-to-resource.go (3)
30-31: Enhance error messages and reduce duplicationThe current implementation duplicates the error message between logging and return. Consider making the messages more specific and using a helper function to reduce duplication.
Here's a suggested improvement:
- log.Error("job is required") - return fmt.Errorf("job is required") + msg := "job ID flag is required and must be a valid UUID" + log.Error(msg) + return fmt.Errorf(msg) - log.Error("resource is required") - return fmt.Errorf("resource is required") + msg := "resource identifier flag is required and must not be empty" + log.Error(msg) + return fmt.Errorf(msg)Also applies to: 35-36
47-47: Add more context to error logsWhile the error logging is functional, it could be more helpful for debugging by including additional context.
Here's a suggested improvement:
- log.Error("failed to create relationship API client", "error", err) + log.Error("failed to create relationship API client", "error", err, "api_url", apiURL) - log.Error("failed to parse job id", "error", err) + log.Error("failed to parse job id", "error", err, "job_id", jobId) - log.Error("failed to create job to resource relationship", "error", err) + log.Error("failed to create job to resource relationship", + "error", err, + "job_id", jobId, + "resource", resourceIdentifier)Also applies to: 53-54, 62-63
Line range hint
1-78: Consider enhancing the logging implementationThe current implementation only logs errors, but adding the following improvements would make the logging more comprehensive:
- Add success logging to track successful operations
- Add debug logs for important steps
- Consider adding log configuration (level, format, output)
Example implementation:
func NewCreateRelationshipCmd() *cobra.Command { + // Add at the start of RunE + log.Debug("creating job-to-resource relationship", + "job_id", jobId, + "resource", resourceIdentifier) + // Add before return in RunE + log.Info("successfully created job-to-resource relationship", + "job_id", jobId, + "resource", resourceIdentifier)Consider adding a logging configuration function at the CLI root level to ensure consistent logging across all commands.
cmd/ctrlc/root/api/create/relationship/resourcetoresource/resource-to-resource.go (2)
Line range hint
68-86: Enhance error logging with additional contextWhile the structured logging is good, we can improve debugging by adding more context to the log messages.
-log.Error("failed to create relationship API client", "error", err) +log.Error("failed to create relationship API client", "error", err, "api_url", apiURL) -log.Error("failed to parse workspace id", "error", err) +log.Error("failed to parse workspace id", "error", err, "workspace_id", workspaceId) -log.Error("failed to create resource to resource relationship", "error", err) +log.Error("failed to create resource to resource relationship", "error", err, "workspace_id", workspaceId, "from", fromIdentifier, "to", toIdentifier, "type", relationshipType)
Line range hint
31-86: Consider adding different log levelsCurrently, all logs are at ERROR level. Consider using:
- ERROR: For actual errors that prevent operation (API failures, parsing errors)
- WARN: For validation issues (missing fields, invalid types)
- DEBUG: For operation details (successful API calls, parsed values)
This would provide better log filtering capabilities in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/ctrlc/root/api/create/relationship/jobtoresource/job-to-resource.go(3 hunks)cmd/ctrlc/root/api/create/relationship/resourcetoresource/resource-to-resource.go(4 hunks)
🔇 Additional comments (2)
cmd/ctrlc/root/api/create/relationship/jobtoresource/job-to-resource.go (1)
6-7: LGTM! Good choice of logging library
The charmbracelet/log package is a solid choice for CLI applications, offering structured logging with good terminal output formatting.
cmd/ctrlc/root/api/create/relationship/resourcetoresource/resource-to-resource.go (1)
7-7: LGTM: Good choice of logging library
The charmbracelet/log library is a solid choice for structured logging.
| log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | ||
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) |
There was a problem hiding this comment.
Fix incorrect logging format
The log.Error call uses printf-style formatting which isn't supported by charmbracelet/log. Use structured logging instead.
-log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType)
+log.Error("invalid relationship type", "type", relationshipType, "allowed", []string{"associated_with", "depends_on"})📝 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.
| log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| log.Error("invalid relationship type", "type", relationshipType, "allowed", []string{"associated_with", "depends_on"}) | |
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) |
| log.Error("workspace is required") | ||
| return fmt.Errorf("workspace-id is required") | ||
| } | ||
|
|
||
| if fromIdentifier == "" { | ||
| log.Error("from is required") | ||
| return fmt.Errorf("from is required") | ||
| } | ||
|
|
||
| if toIdentifier == "" { | ||
| log.Error("to is required") | ||
| return fmt.Errorf("to is required") | ||
| } | ||
|
|
||
| if relationshipType == "" { | ||
| log.Error("type is required") | ||
| return fmt.Errorf("type is required") | ||
| } | ||
|
|
||
| if relationshipType != "associated_with" && relationshipType != "depends_on" { | ||
| log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | ||
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | ||
| } | ||
|
|
||
| if fromIdentifier == toIdentifier { | ||
| log.Error("from and to cannot be the same") | ||
| return fmt.Errorf("from and to cannot be the same") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error logging format
The error logging style is inconsistent across validations. Some use structured logging with fields (like line 68) while others don't. Let's standardize on structured logging throughout.
-log.Error("workspace is required")
+log.Error("missing required field", "field", "workspace-id")
-log.Error("from is required")
+log.Error("missing required field", "field", "from")
-log.Error("to is required")
+log.Error("missing required field", "field", "to")
-log.Error("type is required")
+log.Error("missing required field", "field", "type")
-log.Error("from and to cannot be the same")
+log.Error("invalid field values", "error", "source and target identifiers must be different", "from", fromIdentifier)📝 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.
| log.Error("workspace is required") | |
| return fmt.Errorf("workspace-id is required") | |
| } | |
| if fromIdentifier == "" { | |
| log.Error("from is required") | |
| return fmt.Errorf("from is required") | |
| } | |
| if toIdentifier == "" { | |
| log.Error("to is required") | |
| return fmt.Errorf("to is required") | |
| } | |
| if relationshipType == "" { | |
| log.Error("type is required") | |
| return fmt.Errorf("type is required") | |
| } | |
| if relationshipType != "associated_with" && relationshipType != "depends_on" { | |
| log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| } | |
| if fromIdentifier == toIdentifier { | |
| log.Error("from and to cannot be the same") | |
| return fmt.Errorf("from and to cannot be the same") | |
| log.Error("missing required field", "field", "workspace-id") | |
| return fmt.Errorf("workspace-id is required") | |
| } | |
| if fromIdentifier == "" { | |
| log.Error("missing required field", "field", "from") | |
| return fmt.Errorf("from is required") | |
| } | |
| if toIdentifier == "" { | |
| log.Error("missing required field", "field", "to") | |
| return fmt.Errorf("to is required") | |
| } | |
| if relationshipType == "" { | |
| log.Error("missing required field", "field", "type") | |
| return fmt.Errorf("type is required") | |
| } | |
| if relationshipType != "associated_with" && relationshipType != "depends_on" { | |
| log.Error("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| return fmt.Errorf("type must be either 'associated_with' or 'depends_on', got %s", relationshipType) | |
| } | |
| if fromIdentifier == toIdentifier { | |
| log.Error("invalid field values", "error", "source and target identifiers must be different", "from", fromIdentifier) | |
| return fmt.Errorf("from and to cannot be the same") |
Summary by CodeRabbit
New Features
Bug Fixes