-
Notifications
You must be signed in to change notification settings - Fork 39
AML-4 Algo CLI Auth issue #111
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
Conversation
Algorithmia/CLI.py
Outdated
@@ -354,14 +358,18 @@ def getAPIkey(self,profile): | |||
config_dict = toml.load(key) | |||
apikey = None | |||
if('profiles' in config_dict.keys() and profile in config_dict['profiles'].keys()): | |||
apikey = config_dict['profiles'][profile]['api_key'] | |||
if config_dict['profiles'][profile]['api_key'] != "": |
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.
This could be an and
clause in the previous if.
client = Algorithmia.client(CLI().getAPIkey(profile), CLI().getAPIaddress(profile), CLI().getCert(profile)) | ||
result2 = CLI().ls("data://.my", client) | ||
|
||
self.assertTrue(result2 != "") |
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.
Be more specific about checking the result (not empty is a pretty loose test)
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.
Some nits, overall looks fine.
It was discovered that the
algo auth
command bundled into the CLI will fail if the API Address is any cluster other than the Marketplace. This was isolated to line 34 of the CLI.py file and a test was added to (hopefully) ensure we catch issues like that in the future.