-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[KeyVault] KeyVault Round 3 Commands #1215
Conversation
key import
key backup
key restore
certificate import
certificate pending merge
|
Group Helpkeyvault key
keyvault certificate
keyvault certificate pending
|
Already see a few params that need help text and some command help cleanup. Also, I plan to do a complete sweep for completers and ids in Round 4 so please hold those 😁 |
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.
Added a few small comments
@@ -17,6 +17,7 @@ def extract_full_summary_from_signature(operation): | |||
summary = lines[:match.regs[0][0]] | |||
else: | |||
summary = lines | |||
summary = summary.replace('\n', ' ').replace('\r', '') |
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.
Since this affects the 'core' module, what's the impact of this change / why?
Want to know if it will adversely affect other modules.
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.
The first sentence is used as the short summary. If the first sentence has a newline, it would be preserved which resulted in some wonky output. This strips out the newlines the same way the long-summary does. So it will affect other modules, but in a positive way.
@@ -61,6 +67,8 @@ def validate_key_type(ns): | |||
'hsm': 'RSA-HSM' | |||
} | |||
ns.destination = dest_to_type_map[ns.destination] | |||
if ns.destination == 'RSA' and hasattr(ns, 'byok_file') and ns.byok_file: | |||
raise CLIError('BYOK keys are hardward protected. Omit --protection') |
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.
hardward
?
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 be hardware
def download_secret(client, vault_base_url, secret_name, file_path, file_encoding='utf8', | ||
secret_version='', decode_binary=None): | ||
secret = client.keyvault.get_secret(vault_base_url, secret_name, secret_version) | ||
raise CLIError('TODO: implement') |
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.
Just checking this is intended?
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.
Yes, this was intended. I was going to finish these commands in Round 3, but decided to defer them to Round 4 because the secret set command needs some additional convenience arguments added in tandem.
This PR adds the following KeyVault commands and associated tests: