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

auth: unit-test for authStore.AuthDisable() #7257

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

Rushit
Copy link
Contributor

@Rushit Rushit commented Feb 1, 2017

This will cover unit-test for AuthDisable in store.go

/cc @mitake @xiang90

t.Errorf("expected %v, got %v", ErrAuthNotEnabled, err)
}

// Disabling disabled auth again and trying to Authenticate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment about why we want to try disabling auth again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling disabled auth to make sure it can return safely if store is already disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rushit Yea. Probably add the comment into the code as a comment? I know why we need to test it. But it just reads strange without an explanation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rushit Great! thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@xiang90
Copy link
Contributor

xiang90 commented Feb 1, 2017

LGTM. Defer to @mitake

This will cover unit-test for AuthDisable in store.go
@mitake
Copy link
Contributor

mitake commented Feb 1, 2017

lgtm, thanks @Rushit

@xiang90 BTW, can I ignore jenkins-proxy-ci? I couldn't see something is going under this PR (the link of Detailed returned 404)

@xiang90
Copy link
Contributor

xiang90 commented Feb 1, 2017

can I ignore jenkins-proxy-ci?

ignore it for now.

@xiang90 xiang90 merged commit 0df1822 into etcd-io:master Feb 1, 2017
@Rushit Rushit deleted the auth_test branch February 1, 2017 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants