-
Notifications
You must be signed in to change notification settings - Fork 18
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
ENT-11134: proper message when invalid cloud credentials #82
Conversation
cf_remote/spawn.py
Outdated
if vm is not None: | ||
ret.append(vm) | ||
if spawned_cb is not None: | ||
spawned_cb() |
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.
Was the removal of vm
as argument to spawned_cb()
on purpose?
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.
mistake 😅
cf_remote/commands.py
Outdated
@@ -403,6 +403,9 @@ def spawn( | |||
role=role, | |||
spawned_cb=print_progress_dot, | |||
) | |||
if len(vms) == 0: |
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.
You could do
if len(vms) == 0: | |
if not vms: |
since empty list evaluates to False
, but maybe your way is more readable 🤔
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.
>>> if not []:
... print("Empty")
... else:
... print("Not empty")
...
Empty
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.
I think maybe your way is clearer 😉
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.
😊
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.
You've updated the function to return None, this won't work;
$ python3
Python 3.11.6 (main, Oct 2 2023, 20:14:46) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> len(None)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: object of type 'NoneType' has no len()
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.
@olehermanse This function is called by spawn_vms
function, where it checks if it's None, and adds to an array
cf_remote/spawn.py
Outdated
print("\nInvalid credentials, check cloud_config.json file, details: \n" + str(error)) | ||
return None |
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.
Same as in other PR. This function already uses raise ValueError
to handle erors. It seems to me you should also use that here, unless you have a good reason not to.
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.
Should I set tracebacklimit
to 0 so the user is not bothered with a stack trace?
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.
no; discussed the better way to do this in person.
32a0b22
to
9ba3f04
Compare
Ticket: ENT-11134 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
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.
Looks good now, thanks!
New invalid credentials error message:
Ticket: ENT-11134
Changelog: None