-
Notifications
You must be signed in to change notification settings - Fork 63
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
Added cli compatibility check #206
base: master
Are you sure you want to change the base?
Conversation
@@ -20,10 +20,12 @@ def get_user_auth_token_by_login(username, password): | |||
""" | |||
Returns user auth token by login. | |||
""" | |||
from evalai.utils.requests import check_compatibility |
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.
@DeeJayBro can we place this import with all other imports at the start of the file
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.
Nope, because it would lead to circular imports
tests/test_requests.py
Outdated
@@ -1,4 +1,9 @@ | |||
import json | |||
from types import SimpleNamespace |
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.
@DeeJayBro Can you move this import to line number 21
evalai/utils/requests.py
Outdated
if semver.compare(version, response.headers["Minimum-CLI-Version"]) is -1: | ||
echo( | ||
style( | ||
"\nYour CLI is too old and needs to be updated from {0} to {1}\n" |
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.
Hi!
Can you implement this comment as well: #201 (comment)
@vkartik97 For some reason he did find semver in https://travis-ci.org/Cloud-CV/evalai-cli/builds/624937908 but not in https://travis-ci.org/Cloud-CV/evalai-cli/builds/625048319 |
Restarting the build seemed to solve it |
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.
@DeeJayBro can you resolve the merge conflicts. Please let me know if you face any issues in resolving conflicts
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 👍
Thanks for the contribution
Inserted function to check if the cli version is still supported by the server