Skip to content

fix: Use log library #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 25, 2024
Merged

fix: Use log library #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions cmd/ctrlc/root/api/create/relationship/jobtoresource/job-to-resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package jobtoresource
import (
"fmt"

"github.com/charmbracelet/log"

"github.com/MakeNowJust/heredoc/v2"
"github.com/ctrlplanedev/cli/internal/api"
"github.com/ctrlplanedev/cli/internal/cliutil"
Expand All @@ -25,11 +27,13 @@ func NewCreateRelationshipCmd() *cobra.Command {
`),
PreRunE: func(cmd *cobra.Command, args []string) error {
if jobId == "" {
return fmt.Errorf("job-id is required")
log.Error("job is required")
return fmt.Errorf("job is required")
}

if resourceIdentifier == "" {
return fmt.Errorf("resource-identifier is required")
log.Error("resource is required")
return fmt.Errorf("resource is required")
}

return nil
Expand All @@ -40,20 +44,23 @@ func NewCreateRelationshipCmd() *cobra.Command {

client, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey)
if err != nil {
log.Error("failed to create relationship API client", "error", err)
return fmt.Errorf("failed to create relationship API client: %w", err)
}

jobIdUUID, err := uuid.Parse(jobId)
if err != nil {
return fmt.Errorf("failed to parse job-id: %w", err)
log.Error("failed to parse job id", "error", err)
return fmt.Errorf("failed to parse job id: %w", err)
}

resp, err := client.CreateJobToResourceRelationship(cmd.Context(), api.CreateJobToResourceRelationshipJSONRequestBody{
JobId: jobIdUUID,
ResourceIdentifier: resourceIdentifier,
})
if err != nil {
return fmt.Errorf("failed to create job-to-resource relationship: %w", err)
log.Error("failed to create job to resource relationship", "error", err)
return fmt.Errorf("failed to create job to resource relationship: %w", err)
}

return cliutil.HandleOutput(cmd, resp)
Expand Down
14 changes: 12 additions & 2 deletions cmd/ctrlc/root/api/create/relationship/resourcetoresource/resource-to-resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/MakeNowJust/heredoc/v2"
"github.com/charmbracelet/log"
"github.com/ctrlplanedev/cli/internal/api"
"github.com/ctrlplanedev/cli/internal/cliutil"
"github.com/google/uuid"
Expand All @@ -27,26 +28,32 @@ func NewCreateRelationshipCmd() *cobra.Command {
`),
PreRunE: func(cmd *cobra.Command, args []string) error {
if workspaceId == "" {
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)
Comment on lines +51 to 52
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)

}

if fromIdentifier == toIdentifier {
log.Error("from and to cannot be the same")
return fmt.Errorf("from and to cannot be the same")
Comment on lines +31 to 57
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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")

}

Expand All @@ -58,12 +65,14 @@ func NewCreateRelationshipCmd() *cobra.Command {

client, err := api.NewAPIKeyClientWithResponses(apiURL, apiKey)
if err != nil {
log.Error("failed to create relationship API client", "error", err)
return fmt.Errorf("failed to create relationship API client: %w", err)
}

workspaceIdUUID, err := uuid.Parse(workspaceId)
if err != nil {
return fmt.Errorf("failed to parse workspace-id: %w", err)
log.Error("failed to parse workspace id", "error", err)
return fmt.Errorf("failed to parse workspace id: %w", err)
}

resp, err := client.CreateResourceToResourceRelationship(cmd.Context(), api.CreateResourceToResourceRelationshipJSONRequestBody{
Expand All @@ -73,7 +82,8 @@ func NewCreateRelationshipCmd() *cobra.Command {
Type: relationshipType,
})
if err != nil {
return fmt.Errorf("failed to create resource-to-resource relationship: %w", err)
log.Error("failed to create resource to resource relationship", "error", err)
return fmt.Errorf("failed to create resource to resource relationship: %w", err)
}

return cliutil.HandleOutput(cmd, resp)
Expand Down