-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[KeyVault] KeyVault Round 4 Commands #1275
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
|
secret set secret download certificate download |
derekbekoe
left a comment
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 overall. Added a few comments..
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 like this is unused?
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.
Consider checking for a directory as well as you can't write to a file_path if it's actually a directory.
if os.path.isfile(file_path) or os.path.isdir(file_path):
...
https://docs.python.org/2/library/os.path.html#os.path.isdir
Then, the error message would have "File or directory already exists".
Just showing you can't have a file and directory with the same name.
Dereks-MacBook-Pro-2:atest debekoe$ ls -la
total 0
drwxr-xr-x 2 debekoe staff 68 Nov 11 13:42 .
drwxr-xr-x+ 63 debekoe staff 2142 Nov 11 13:42 ..
Dereks-MacBook-Pro-2:atest debekoe$ mkdir a_directory
Dereks-MacBook-Pro-2:atest debekoe$ ls -la
total 0
drwxr-xr-x 3 debekoe staff 102 Nov 11 13:42 .
drwxr-xr-x+ 63 debekoe staff 2142 Nov 11 13:42 ..
drwxr-xr-x 2 debekoe staff 68 Nov 11 13:42 a_directory
Dereks-MacBook-Pro-2:atest debekoe$ echo 'helloworld' > a_directory
-bash: a_directory: Is a directory
Dereks-MacBook-Pro-2:atest debekoe$ echo $?
1
Dereks-MacBook-Pro-2:atest debekoe$
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 here
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 catch the exception which is fine but should you then raise CLIError after you remove the file?
Otherwise I think the command would return an exit code of 0 and it'd look like it downloaded fine.
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 here.
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.
nit: i.e. or e.g.?
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'm thinking e.g. as you're listing examples.
|
@derekbekoe fixed up your issues. |
Closes #885.
This PR updates the
secret setcommand and adds thesecret downloadandcertificate downloadcommands. With this, the entire KeyVault data plane functionality should be exposed.Plan to defer merging this until #1059 is merged.