chainloop icon indicating copy to clipboard operation
chainloop copied to clipboard

improve how we show validation errors

Open migmartri opened this issue 1 year ago • 3 comments

We are currently showing validations errors like this excerpt from https://github.com/chainloop-dev/chainloop/issues/1206

chainloop workflow create --name build --project xy --team abc
ERR failed to create workflow: rpc error: code = InvalidArgument desc = validation error: name already taken

we should ideally show just

validation error: name already taken

in theory we have some error handling in the cli main.go file but it doesn't seem to be working for this specific error type.

migmartri avatar Aug 12 '24 13:08 migmartri

the problem is that our code in main.go can not extract the error information from the gRPC error because it's not identified as such. The reason is that sometimes we wrap the grpc errors into another errors and hence it breaks the detection.

This is an example of a place where we do that https://github.com/chainloop-dev/chainloop/blob/a85cf811ad804b48564143ac427d62e5b2420d08/app/cli/cmd/organization_apitoken_create.go#L45, causing the error to be shown like this

go run main.go --insecure org api-token create --name test
WRN API contacted in insecure mode
ERR creating API token: creating API token: rpc error: code = AlreadyExists desc = duplicated: name already taken

but if we return a plain err

diff --git a/app/cli/cmd/organization_apitoken_create.go b/app/cli/cmd/organization_apitoken_create.go
index 012663bb..8f8573e5 100644
--- a/app/cli/cmd/organization_apitoken_create.go
+++ b/app/cli/cmd/organization_apitoken_create.go
@@ -42,7 +42,7 @@ func newAPITokenCreateCmd() *cobra.Command {
 
                        res, err := action.NewAPITokenCreate(actionOpts).Run(context.Background(), name, description, duration)
                        if err != nil {
-                               return fmt.Errorf("creating API token: %w", err)
+                               return err
                        }
 
                        if flagOutputFormat == "token" {
diff --git a/app/cli/internal/action/apitoken_create.go b/app/cli/internal/action/apitoken_create.go
index 7c3a7b58..0eba058b 100644
--- a/app/cli/internal/action/apitoken_create.go
+++ b/app/cli/internal/action/apitoken_create.go
@@ -18,7 +18,6 @@ package action
 import (
        "context"
        "errors"
-       "fmt"
        "time"
 
        pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1"
@@ -43,7 +42,7 @@ func (action *APITokenCreate) Run(ctx context.Context, name, description string,
 
        resp, err := client.Create(ctx, req)
        if err != nil {
-               return nil, fmt.Errorf("creating API token: %w", err)
+               return nil, err
        }
 
        p := resp.Result
~
~
~

the error looks like this

go run main.go --insecure org api-token create --name test
WRN API contacted in insecure mode
ERR duplicated: name already taken

ideally we should be able to find gRPC errors inside wrapped errors but I am not sure it's possible, otherwise we should just make sure to return naked gRPC errors.

migmartri avatar Aug 12 '24 21:08 migmartri

Discussion on this matter here https://github.com/grpc/grpc-go/issues/2934 where I am extracting two interesting data-points.

  • prometheus workaround https://github.com/grpc-ecosystem/go-grpc-prometheus/pull/84
  • possible implementation already present? https://github.com/grpc/grpc-go/pull/6031

migmartri avatar Aug 12 '24 21:08 migmartri

so we have that code and seems to be working, wrapped gRPC errors are properly returned to their status counterparts, the issue though is that if they come wrapped, the message contains the whole error chain.

https://github.com/grpc/grpc-go/blob/ced812e3287e15a009eab5b271c25750050a2f82/status/status.go#L123

and it seems to be overridden, I do not think there is a way to get the original message.

migmartri avatar Aug 12 '24 21:08 migmartri