Skip to content

Conversation

@isra-fel
Copy link
Member

@isra-fel isra-fel commented Nov 27, 2020

Description

According to our discussion with KeyVault team, here's the summary of the changes:
(I organized code changes in different commits. I suggest review them one by one.)

  1. Management plane
    • Rename, add "KeyVault" prefix
    • Old: New-AzManagedHsm
    • New: New-AzKeyVaultManagedHsm
  2. Key operations
    • Merge with existing "KeyVaultKey" cmdlets
    • Make -VaultName and -HsmName exclusive for different scenarios
    • Old: Add-AzKeyVaultKey and Add-AzManagedHsmKey
    • New: Merge both into Add-AzKeyVaultKey
  3. RBAC
    • Rename, use "KeyVault" as prefix
    • Old: Get-AzManagedHsmRoleDefinition
    • New: Get-AzKeyVaultRoleDefinition
  4. Full backup & restore
    • Rename, use "KeyVault" as prefix
    • Old: Backup-AzManagedHsm
    • New: Backup-AzKeyVault
  5. Security domain
    • Rename, use "KeyVault" as prefix
    • Use verb Export and Import
    • Old: Backup-AzManagedHsmSecurityDomain and Restore-AzManagedHsmSecurityDomain
    • New: Export-AzKeyVaultSecurityDomain and Import-AzKeyVaultSecurityDomain

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@msJinLei
Copy link
Contributor

msJinLei commented Dec 1, 2020

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

if (existingResource == null)
{
throw new Exception(string.Format("A managed HSM with name '{0}' in resource group '{1}' does not exist. Please use New-AzManagedHsm to create a managed HSM with these properties.", this.Name, this.ResourceGroupName));
throw new Exception(string.Format("A managed HSM with name '{0}' in resource group '{1}' does not exist. Please use New-AzKeyVaultManagedHsm to create a managed HSM with these properties.", this.Name, this.ResourceGroupName));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It better to use resource string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I'll fix it.

}
}

private void NormalizeParameterSets()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put NormalizeParameterSets into base class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I could, but the logic of this method is slightly different in every cmdlet.
For example, some cmdlets have -ResourceId and some don't, some even have -HsmResourceId as well.
I'm afarid we won't get much benifit to write it in base class.

@isra-fel isra-fel added this to the S179 (2020-12-08) milestone Dec 1, 2020
@isra-fel isra-fel merged commit 4b21b49 into master Dec 1, 2020
@isra-fel isra-fel deleted the yeming/hsm-rename branch December 1, 2020 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants