-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
add Refresh() to mock passwordConnector #1245
Conversation
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.
one nitpick, lgtm
connector/mock/connectortest.go
Outdated
@@ -112,3 +112,7 @@ func (p passwordConnector) Login(ctx context.Context, s connector.Scopes, userna | |||
} | |||
|
|||
func (p passwordConnector) Prompt() string { return "" } | |||
|
|||
func (p passwordConnector) Refresh(ctx context.Context, s connector.Scopes, identity connector.Identity) (connector.Identity, error) { |
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.
[nit] if ctx
and s
aren't used, let's go with
func (p passwordConnector) Refresh(_ context.Context, _ connector.Scopes, identity connector.Identity) (connector.Identity, error) {
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.
@srenatus updated
5180dcb
to
f8e0db6
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.
LGTM
@srenatus any chance you're able to merge this now that the repo has moved? |
@scotthew1 I don't see why not, indeed. Just one more nitpick: Could you add something like _ connector.RefreshConnector = passwordConnector{} next to that one, since that's what the addition does, make the mock connector satisfy |
f8e0db6
to
2707302
Compare
@srenatus updated. also rebased on master |
Status update? |
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.
Thank you! 🎉
add Refresh() to mock passwordConnector
we've been using the mock passwordConnector for some internal testing, but it doesn't satisfy the
RefreshConnector
interface. this change fixes that.