Bugfix: make profile name required for delete #175
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
|
|
||
| def delete_profile(profile_name): | ||
| profile = _get_profile(profile_name) | ||
| profile_name = profile.name |
There was a problem hiding this comment.
How would these get out of sync?
There was a problem hiding this comment.
If you call _get_profile() with None it uses the default profile, and as a hacker, I actually use this individual module in other non-CLI related scripts (creating SDKs from CLI profiles for py42 related stuff).
I don't see anything bad that would happen because of this change in the CLI
There was a problem hiding this comment.
It makes sense from the command perspective to not be allowed to delete the default profile, but from this module's perspective, if you are grabbing the profile from the name, you should use the name of the profile object because that is the real profile name you are working with
There was a problem hiding this comment.
If you look at other methods in this module, that is what happens too, so this is consistent with that
There was a problem hiding this comment.
Ok cool, I was just wondering if you had uncovered a bug caused by this, to get a sense of what the change did.
I do see that if you just call code42 profile delete without a profile name, it errors out. This change allows it to delete the default, but then it prints the name wrong since we're just printing the argument passed:
# code42 profile delete
Deleting this profile will also delete any stored passwords and checkpoints. Are you sure? (y/n): y
Profile 'None' has been deleted.
It also doesn't print the "<profile_name> is currently the default profile!" message in the warning..
A simple fix would be to just make the profile_name arg required on just the delete command.. Thoughts?
There was a problem hiding this comment.
Oh ok, yes I had thought that it was required. If you are not allowed to delete the default profile, it really should be required. Are you sure that it is this change allows this error? What happens on master branch? There might actually be a bug related to this. Awesome that this was uncovered. I am going to look tomorrow.
There was a problem hiding this comment.
Yes I verified that this bug is indeed already on Master, so I will fix it as an adittion to this PR
antazoey
changed the title
Adds a missing test
Safer delete profile method