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

Fix #205: Add functionality to reset authentication token and logout #233

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

nikochiko
Copy link
Contributor

@nikochiko nikochiko commented Dec 28, 2019

Address #205

Objective:

To allow user to easily log out (i.e. reset existing authentication token) from the command line.

Changes proposed:

  1. Add reset_user_auth_token function in auth utils
  2. Add tests for reset_user_auth_token
  3. Add new command to logout:
evalai logout

4, Add functionality in set_token command to clear existing token with:

evalai set_token clear_token

Edit: Removed 4: functionality to clear existing token from set_token command.


@RishabhJain2018 @vkartik97 @Ram81 Please review.

Copy link
Member

@krtkvrm krtkvrm left a comment

Choose a reason for hiding this comment

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

@nikochiko
With respect to changes proposed in this PR:

4, Add functionality in set_token command to clear existing token with:
evalai set_token clear_token

The use of evalai set_token just to set the token. To put it in other words, responsibility set_token is just to set the token. You are delegating it one more task to clear token using flag tokens. How about clear token only using logout command? I am trying to use Single Responsibility Principal to explain this.

@nikochiko
Copy link
Contributor Author

@vkartik97 Got it. Yes now that I think about it, it makes a lot of sense. I will make that change.
P.S. Thanks for introducing this to me, will keep in mind! 👍

@nikochiko
Copy link
Contributor Author

nikochiko commented Dec 28, 2019

@vkartik97 Any comments on the rest of the task?
I have tried to make it consistent with the rest of the project, but the tests for utils was a new part. So what are your thoughts on the location of tests for auth utils? Currently, it is placed along with the other tests but as we will need tests for more utils (e.g. requests utils), should we create a new folder?

self.assertEqual(value, expected)

@mock.patch("evalai.utils.auth.os.remove")
def test_reset_user_auth_token_when_writing_to_file_fails(self, mock_remove):

Choose a reason for hiding this comment

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

@nikochiko What does this test signify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pushkalkatara this is the test for the case when file deleting fails. And oops I think I named it incorrectly 😕 . How about changing the name to test_reset_user_auth_token_when_token_file_removal_fails ?

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! 👍

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

Successfully merging this pull request may close these issues.

3 participants