-
Notifications
You must be signed in to change notification settings - Fork 24
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
refactor(auth): replace use of deprecated ErrOTPRequired
#1009
refactor(auth): replace use of deprecated ErrOTPRequired
#1009
Conversation
ErrOTPRequired
ba73dcc
to
7dade04
Compare
@@ -11,12 +11,12 @@ import ( | |||
"time" | |||
|
|||
"github.com/pkg/errors" | |||
errgo "gopkg.in/errgo.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this dependency from this file and it had a lot of side effects, sorry.. 😅
c3148c3
to
2d32d21
Compare
2d32d21
to
5dde6ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some improvements possible but otherwise LGTM
config/auth.go
Outdated
@@ -43,7 +43,7 @@ func Auth(ctx context.Context) (*scalingo.User, string, error) { | |||
if err == nil { | |||
break | |||
} else if scalingoerrors.RootCause(err) == io.EOF { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue(non-blocking): Can you change the deprecated function .RootCause()
to the function .Is()
@@ -11,12 +11,12 @@ import ( | |||
"time" | |||
|
|||
"github.com/pkg/errors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Is it possible to replace all existing errors
to scalingoerrors
?
It will freed us from the use of another deprecated package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible. We declare some public variable, for instance on line 24:
var (
ErrAuthenticationFailed = errors.New("authentication failed")
)
scalingoerrors
requires a context and we have no context available here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM then 👍
Related to Scalingo/go-scalingo#360
Fix Scalingo/go-scalingo#324
Fix Scalingo/go-scalingo#326