evalai-cli icon indicating copy to clipboard operation
evalai-cli copied to clipboard

Add command to setup the CLI

Open nikochiko opened this issue 5 years ago • 8 comments

Changes proposed in this PR:

  • Update login.py to allow calling from another command
  • Added command to set up the CLI configuration like host and auth token
  • Command example:
   evalai setup -u USER -p PASSWORD [-h HOST]
  • Added unit tests for the above command still in progress, but thought I could gain some feedback on the main code in the meanwhile:

P.S.: I tried using a fancy name and showing EvalAI in Text Art form to make it a little interesting 😅 . Please let me know if that needs to be changed.

Mentors: @vkartik97 @pushkalkatara @Ram81 @RishabhJain2018 @yashdusing

nikochiko avatar Dec 18 '19 10:12 nikochiko

Also want to point this out: evalai/login.py#L20-L33 :

This block can be replaced with the set_token command to maintain consistency and following DRY.

Also, there is a possible bug in the code: the except block doesn't end with sys.exit(1) . Hence, the final "Logged in successfully!" will be printed even if the code threw an OSError/IOError.

nikochiko avatar Dec 18 '19 10:12 nikochiko

Ah, pardon me for the mess of commits here. I should've squashed and merged 😅 .

Anyways, a lot of changes proposed in this PR :smile::

  • Refactor code so that multiple commands can use the same piece of code.
  • Removed the previous dependency on other commands, instead both use same helper functions to maintain similar error logs.
  • Added exit codes at a few place.
  • Added unit tests for everything new (setup command, refactored auth utils)

This is in addition to the above description ☝️

Thanks!

nikochiko avatar Dec 19 '19 19:12 nikochiko

@yashdusing the new command is completely covered (coveralls report). The reason for the coverage going down is the part I refactored for usage in multiple places. I am working on it right now and will update the PR with the tests in a few hours!

nikochiko avatar Dec 20 '19 09:12 nikochiko

@yashdusing Fixed both issues! 😄

nikochiko avatar Dec 20 '19 10:12 nikochiko

LGTM 👍 @Ram81 @pushkalkatara @vkartik97 @RishabhJain2018 please review

yashdusing avatar Dec 20 '19 10:12 yashdusing

@nikochiko could you please attach a link to a gif to the PR demonstrating the new command ?

yashdusing avatar Dec 21 '19 06:12 yashdusing

@yashdusing Made the gif: Successful with prompt: setup-demo

When login fails, revert message: evalai-demo-2

nikochiko avatar Dec 21 '19 08:12 nikochiko

@vkartik97 Done! 😄 The new line slipped in during the merging 😸 Fixed it now!

nikochiko avatar Dec 28 '19 15:12 nikochiko