Remove github.com/zeebo/errs dependency #5716
Conversation
We don't really use this dependency for much other than to group some errors together with a common error message prefix. The same can now be accomplished with a couple custom error types and the `errors` standard library package. This package also wasn't consistently adopted throughout the project, so at this point it's probably better to just rely on the standard library functionality, since it's sufficient for the project's use cases. Signed-off-by: Ryan Turner <ryan.turner253@icloud.com>
rturner3
requested review from
evan2645,
amartinezfayo,
azdagron and
MarcosDY
as code owners
Signed-off-by: Ryan Turner <ryan.turner253@icloud.com>
| } | ||
| if c.JWTIssuer != "" { | ||
| jwtIssuer, err := url.Parse(c.JWTIssuer) | ||
| if err != nil || jwtIssuer.Scheme == "" || jwtIssuer.Host == "" { | ||
| return nil, errs.New("the jwt_issuer url could not be parsed") | ||
| return nil, errors.New("the jwt_issuer url could not be parsed") |
There was a problem hiding this comment.
Could we also include the error in here and maybe the jwtIssuer?
There was a problem hiding this comment.
if that is the case, we may need to have 2 if one for err != nil and another for issuer
| @@ -300,7 +300,7 @@ func migrateDB(db *gorm.DB, dbType string, disableMigration bool, log logrus.Fie | |||
| dbCodeVersion, err := getDBCodeVersion(*migration) | |||
| if err != nil { | |||
| log.WithError(err).Error("Error getting DB code version") | |||
| return sqlError.New("error getting DB code version: %v", err) | |||
| return newSQLError("error getting DB code version: %v", err) | |||
There was a problem hiding this comment.
Would this be better in any way with newWrappedSQLError(fmt.Errorf("error getting DB code version: %w", err))? I think it might be better for use with errors.Is.
There was a problem hiding this comment.
I'm worried if that we create a wrapped error here, that may be changing current behavior
| @@ -250,6 +250,7 @@ func (s *PluginSuite) TestBundleCRUD() { | |||
|
|
|||
| // fetch non-existent | |||
| fb, err := s.ds.FetchBundle(ctx, "spiffe://foo") | |||
| s.T().Logf("err type: %T", err) | |||
There was a problem hiding this comment.
Is this a debug leftover?
| if err := cs[i].Close(); err != nil { | ||
| errs = errors.Join(errs, err) | ||
| } |
There was a problem hiding this comment.
Join() discards nil error values so you should be able to do:
| if err := cs[i].Close(); err != nil { | |
| errs = errors.Join(errs, err) | |
| } | |
| errs = errors.Join(errs, cs[i].Close()) |
| func newSQLError(fmtMsg string, args ...any) error { | ||
| return &sqlError{ | ||
| msg: fmt.Sprintf(fmtMsg, args...), | ||
| } | ||
| } | ||
|
|
||
| func newWrappedSQLError(err error) error { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| return &sqlError{ | ||
| err: err, | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: can you move private functions to the end of the file?
| return fmt.Sprintf("%s: %s", datastoreSQLErrorPrefix, s.msg) | ||
| } | ||
|
|
||
| func (s *sqlError) Is(err error) bool { |
There was a problem hiding this comment.
it is no clear to me, the use of this function,
this sqlError has 2 "states" when you use msg or when you use err
but here we are only validating msg,
may we validate both?
| assert.True(t, errors.As(err, &sErr)) | ||
|
|
||
| assert.True(t, errors.Is(err, &sqlError{ |
There was a problem hiding this comment.
NIT: use assert.ErrorIs and assert.ErrorAS (same apply to all tests)
| @@ -300,7 +300,7 @@ func migrateDB(db *gorm.DB, dbType string, disableMigration bool, log logrus.Fie | |||
| dbCodeVersion, err := getDBCodeVersion(*migration) | |||
| if err != nil { | |||
| log.WithError(err).Error("Error getting DB code version") | |||
| return sqlError.New("error getting DB code version: %v", err) | |||
| return newSQLError("error getting DB code version: %v", err) | |||
There was a problem hiding this comment.
I'm worried if that we create a wrapped error here, that may be changing current behavior
| } | ||
|
|
||
| if !opts.ParseTime { | ||
| return sqlError.Wrap(errors.New("invalid mysql config: missing parseTime=true param in connection_string")) | ||
| return newWrappedSQLError(errors.New("invalid mysql config: missing parseTime=true param in connection_string")) |
There was a problem hiding this comment.
may we use newSQLError error here?
| } | ||
| if c.JWTIssuer != "" { | ||
| jwtIssuer, err := url.Parse(c.JWTIssuer) | ||
| if err != nil || jwtIssuer.Scheme == "" || jwtIssuer.Host == "" { | ||
| return nil, errs.New("the jwt_issuer url could not be parsed") | ||
| return nil, errors.New("the jwt_issuer url could not be parsed") |
There was a problem hiding this comment.
if that is the case, we may need to have 2 if one for err != nil and another for issuer
We don't really use this dependency for much other than to group some errors together with a common error message prefix. The same can now be accomplished with a couple custom error types and the
errorsstandard library package.This package also wasn't consistently adopted throughout the project, so at this point it's probably better to just rely on the standard library functionality, since it's sufficient for the project's use cases.
Fixes #5631.