-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Enhanced Key Vault Certificate Download and AAD SP Integration #3003
Conversation
az ad sp create-for-rbac
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.
Please fix the style check error and merge
az ad sp create-for-rbac
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.
LGTM. A couple very minor changes but otherwise I think it will be a big improvement.
@@ -6,6 +6,7 @@ Release History | |||
unreleased | |||
++++++++++++++++++++ | |||
|
|||
* Change `az keyvault certificate download` to better reflect the encoding options (PEM and DER). |
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 these options have changed, this is a breaking change, so please label it BC.
@@ -73,6 +73,7 @@ def completer(prefix, action, parsed_args, **kwargs): # pylint: disable=unused-a | |||
json_web_key_op_values = ', '.join([x.value for x in JsonWebKeyOperation]) | |||
secret_encoding_values = secret_text_encoding_values + secret_binary_encoding_values | |||
certificate_file_encoding_values = ['binary', 'string'] |
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 can delete this since it's no longer used.
@@ -506,7 +513,8 @@ def create_service_principal_for_rbac( | |||
'''create a service principal and configure its access to Azure resources | |||
:param str name: a display name or an app id uri. Command will generate one if missing. | |||
:param str password: the password used to login. If missing, command will generate one. | |||
:param str cert: PEM formatted public certificate. Do not include private key info. | |||
:param str cert: string or @file_path PEM formatted public certificate. Do not include private |
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.
Recommend this for consistency for where this is used elsewhere:
PEM formatted public certificate using JSON string or `@<file path>` to load from a file.
Codecov Report
@@ Coverage Diff @@
## master #3003 +/- ##
=========================================
- Coverage 63.17% 63.1% -0.08%
=========================================
Files 484 484
Lines 27582 27621 +39
Branches 4276 4280 +4
=========================================
+ Hits 17426 17431 +5
- Misses 9002 9034 +32
- Partials 1154 1156 +2
Continue to review full report at Codecov.
|
Still LGTM. Feel free to merge when you are happy 😁 |
--cert
az keyvault certificate download
to better reflect the encoding options (PEM and DER).Usage: