Skip to content
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.

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

improve how we show validation errors #1207

Closed
migmartri opened this issue Aug 12, 2024 · 3 comments · Fixed by #1236
Closed

improve how we show validation errors #1207

migmartri opened this issue Aug 12, 2024 · 3 comments · Fixed by #1236

Comments

@migmartri
Copy link
Member

We are currently showing validations errors like this excerpt from #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
Copy link
Member Author

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

return fmt.Errorf("creating API token: %w", err)
, 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
Copy link
Member Author

Discussion on this matter here grpc/grpc-go#2934 where I am extracting two interesting data-points.

@migmartri
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant