[KeyVault] adding recovery feature commands by schaabs · Pull Request #3631 · Azure/azure-cli

schaabs · 2017-06-07T20:35:31Z

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

(see Authoring Command Modules)

…very-feature-updates

troydai · 2017-06-08T03:58:08Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/custom.py

+                                      StoragePermissions.setsas,
+                                      StoragePermissions.listsas,
+                                      StoragePermissions.getsas,
+                                      StoragePermissions.deletesas])


Is there a way to get these options from SDK?

All the permissions enumerations used here are defined in the SDK, we explicitly list out the values because the permissions granted by default are not all available permissions.

troydai · 2017-06-08T03:59:03Z

src/command_modules/azure-cli-role/setup.py

    'azure-mgmt-authorization==0.30.0rc6',
    'azure-graphrbac==0.30.0rc6',
-    'azure-keyvault==0.3.0',
+    'azure-keyvault==0.3.4',


@derekbekoe raise any API profile issue?

Changing the API versions of other command modules may break their tests depending on whether there were breaking changes between the different SDK versions.

there are no breaking changes between azure-keyvault 0.3.0 and 0.3.4

tjprescott

Tests needed for the new commands.

tjprescott · 2017-06-12T17:33:09Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/_params.py

                      help='Allow Resource Manager to retrieve secrets from the vault.',
                      **three_state_flag())
+register_cli_argument('keyvault', 'enable_for_soft_delete',
+                      help='Enable vault deletion recovery for the vault, and all contained entities',


I'd recommend shortening the option name for this to "--enable-soft-delete".

tjprescott · 2017-06-12T17:35:40Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/_params.py


+# purge arguments
+register_cli_argument('keyvault purge', 'vault_name', required=True, completer=None, validator=None)
+register_cli_argument('keyvault purge', 'location', required=True, completer=None, validator=None)


What is the reason for these two registrations?

tjprescott · 2017-06-12T17:39:36Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/commands.py

                                data_client_path.format('KeyVaultClient.get_keys'))
 cli_keyvault_data_plane_command('keyvault key list-versions',
                                data_client_path.format('KeyVaultClient.get_key_versions'))
+cli_keyvault_data_plane_command('keyvault key list-deleted',


Alternatively, you could simply enhance the existing list command with a --deleted flag (or something similar) rather than have a completely separate command. This would be similar to list/list-all folding that many commands do

list and list-deleted have some conflicting arguments in some cases and return different types in all, this is why I created them as separate commands

tjprescott · 2017-06-12T17:44:35Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/custom.py

+backup_secret.__doc__ = KeyVaultClient.backup_secret.__doc__
+
+
+def restore_secret(client, vault_base_url, file_path):


You should add a scenario tests that backs up and restores a secret successfully. From doing the original KV commands, I would be surprised if more edge cases didn't need to be covered on account of Python2 vs 3 and Linux vs Windows.

Backup and restore has been added to the secret scenario

schaabs · 2017-06-16T19:55:30Z

@tjprescott All the commands have tests, and I've made updates based on your comments. PTAL

codecov-io · 2017-06-16T21:11:24Z

Codecov Report

Merging #3631 into master will decrease coverage by <.01%.
The diff coverage is 83.78%.

@@            Coverage Diff             @@
##           master    #3631      +/-   ##
==========================================
- Coverage   72.14%   72.13%   -0.01%     
==========================================
  Files         421      421              
  Lines       26010    26046      +36     
  Branches     3943     3943              
==========================================
+ Hits        18765    18789      +24     
- Misses       6026     6040      +14     
+ Partials     1219     1217       -2

Impacted Files	Coverage Δ
...yvault/azure/cli/command_modules/keyvault/_help.py	`100% <100%> (ø)`	⬆️
...ult/azure/cli/command_modules/keyvault/commands.py	`100% <100%> (ø)`	⬆️
...ault/azure/cli/command_modules/keyvault/_params.py	`86.25% <100%> (+0.1%)`	⬆️
...vault/azure/cli/command_modules/keyvault/custom.py	`69.56% <66.66%> (-1.56%)`	⬇️
.../azure/cli/command_modules/keyvault/_validators.py	`72.18% <0%> (-0.67%)`	⬇️
...nent/azure/cli/command_modules/component/custom.py	`16.23% <0%> (ø)`	⬆️
src/azure-cli-core/azure/cli/core/util.py	`70.06% <0%> (ø)`	⬆️
...dback/azure/cli/command_modules/feedback/custom.py	`31.25% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dff725f...5fef8c6. Read the comment docs.

tjprescott

A couple small changes but otherwise LGTM.

tjprescott · 2017-06-16T21:19:11Z

src/command_modules/azure-cli-keyvault/HISTORY.rst


 Release History
 ===============
+2.0.7 (2017-06-16)


This should just say "unreleased". Derek will fill this in later.

tjprescott · 2017-06-16T21:22:10Z

src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/custom.py

+    :param resource_group_name: The original resource group of the vault to recover
+    :param location: The original location of the vault to recover
+    :return: The properties of the recovered key vault
+    """


This help text should be moved (and reformatted) to the help.py file for the module. This is because the live docs links to help.py if someone wants to make a change to help text. While this way of creating help text is still supported (for SDK reflection) we don't allow it in custom commands for new PRs.

tjprescott · 2017-06-16T21:23:49Z

src/command_modules/azure-cli-keyvault/tests/test_keyvault_commands.py

-
-        byok_key_file = os.path.join(TEST_DIR, 'TestBYOK-NA.byok')
-        self.cmd(
-            'keyvault key import --vault-name {} -n import-key-byok --byok-file "{}"'.format(kv,


I understand why you removed the BYOK test, but why the PEM file import?

tjprescott · 2017-06-16T21:28:38Z

src/command_modules/azure-cli-keyvault/tests/test_keyvault_commands.py

                kv, pfx_plain_file, pfx_policy_path))


+class KeyVaultSoftDeleteScenarioTest(ResourceGroupVCRTestBase):


Ideally this would inherit from ScenarioTest, but I'm fine merging this as is.

Scott Schaab added 3 commits June 7, 2017 13:18

adding recovery feature commands for keyvault command module

615e050

updating history and version

30c940d

Merge branch 'master' of https://github.com/Azure/azure-cli into reco…

8f4abdf

…very-feature-updates

schaabs requested a review from tjprescott June 7, 2017 20:35

msftclas added the cla-not-required label Jun 7, 2017

troydai reviewed Jun 8, 2017

View reviewed changes

tjprescott suggested changes Jun 12, 2017

View reviewed changes

tjprescott added New Command(s) KeyVault az keyvault labels Jun 12, 2017

Scott Schaab added 8 commits June 12, 2017 10:49

fixes and tests for new commands

4d4eb78

test updates and re-recording

8b75082

pylint fixes

8b5aef5

pylint fixes in keyvault tests

c32bb26

more pylint fixes in keyvault tests

8abd1ae

updating batch and vm tests and recordings

a3d20af

removing unneeded params

85bbbe3

fixing cli argument typo

f7f941e

Scott Schaab added 3 commits June 16, 2017 12:58

Merge branch 'master' into recovery-feature-updates

a531fdc

fixing introduce pylint failure

06d3b0f

adding comment as last commit didn't update PR

a7e2044

schaabs changed the title ~~Recovery feature updates~~ [KeyVault] adding recovery feature commands Jun 16, 2017

tjprescott suggested changes Jun 16, 2017

View reviewed changes

Scott Schaab added 2 commits June 16, 2017 15:04

updates from feedback

7134c92

removing docstring from recover_keyvault

c43492f

tjprescott approved these changes Jun 16, 2017

View reviewed changes

removing trailing whitespace

5fef8c6

schaabs merged commit e0ee922 into Azure:master Jun 16, 2017

haroldrandom added the cla-not-required label Oct 25, 2019

haroldrandom added KeyVault az keyvault New Command(s) labels Oct 25, 2019

jiasli mentioned this pull request May 29, 2023

{Keyvault} Remove useless AAD endpoint and honor user input --subscription to support cross sub operations #26539

Merged

3 tasks

		backup_secret.__doc__ = KeyVaultClient.backup_secret.__doc__


		def restore_secret(client, vault_base_url, file_path):

		kv, pfx_plain_file, pfx_policy_path))


		class KeyVaultSoftDeleteScenarioTest(ResourceGroupVCRTestBase):

Conversation

schaabs commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

Command Guidelines

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schaabs Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schaabs commented Jun 16, 2017

Uh oh!

codecov-io commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

schaabs commented Jun 7, 2017 •

edited

Loading

schaabs Jun 16, 2017 •

edited

Loading

codecov-io commented Jun 16, 2017 •

edited

Loading