Skip to content

Conversation

notyashhh
Copy link
Member

@notyashhh notyashhh commented Sep 23, 2025

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

@Copilot Copilot AI review requested due to automatic review settings September 23, 2025 04:55
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Azure KeyVault Management SDK to use API specification version 2025-05-01, enabling support for NetworkRuleSet functionality in ManagedHsm. The change involves updating the AutoRest generation configuration to point to the newer API specification files.

  • Updates API specification version from 2024-11-01 to 2025-05-01
  • Updates the commit hash reference for the Azure REST API specifications
  • Maintains the same input files (common.json, keyvault.json, managedHsm.json) but from the newer API version

@isra-fel
Copy link
Member

isra-fel commented Oct 2, 2025

Converting to draft as it looks not ready for review yet

@isra-fel isra-fel marked this pull request as draft October 2, 2025 07:22
@notyashhh notyashhh marked this pull request as ready for review October 8, 2025 17:00
@isra-fel isra-fel requested a review from Copilot October 8, 2025 22:51
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 107 out of 137 changed files in this pull request and generated 4 comments.

@notyashhh notyashhh requested review from Copilot and isra-fel October 8, 2025 22:54
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 107 out of 137 changed files in this pull request and generated no new comments.

Assert-AreEqual $false $actual.EnabledForDeployment
# Default Access Policy is not set by Service Principal
Assert-AreEqual 0 @($actual.AccessPolicies).Count
Assert-AreEqual 1 @($actual.AccessPolicies).Count
Copy link
Member

Choose a reason for hiding this comment

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

This is actually caused by a feature in the test framework to support recording test cases with user account instead of service account, in which case the command won't add default access policy, so the result is 0. So it's not that the test cases are incorrect, it's just an unfixed behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! In that case should we leave this as 1? (Since from now on, it will use the user account)

Copy link
Member

Choose a reason for hiding this comment

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

I love that you covered both e2e and unit tests.

@VeryEarly VeryEarly self-assigned this Oct 9, 2025
@VeryEarly VeryEarly changed the base branch from main to release-2025-10-14 October 9, 2025 03:59
@VeryEarly VeryEarly merged commit 3f6a211 into release-2025-10-14 Oct 9, 2025
12 checks passed
@VeryEarly VeryEarly deleted the yash/keyvault-mhsm branch October 9, 2025 05:04
Comment on lines +64 to +80
```powershell
Add-AzKeyVaultManagedHsmNetworkRule -Name $myHsm -ResourceGroupName $myRg -IpAddressRange 203.0.113.0/24,198.51.100.10/32 -PassThru
```

```output
Name Resource Group Name Location SKU ProvisioningState Security Domain ActivationStatus
---- ------------------- -------- --- ----------------- --------------------------------
mhsm1814428918 kv-mhsm-rg eastus StandardB1 Succeeded NotActivated
$hsm.OriginalManagedHsm.Properties.NetworkAcls
Bypass : AzureServices
DefaultAction : Deny
IPRules : {203.0.113.0/24, 198.51.100.10/32}
ServiceTags : {}
VirtualNetworkRules : {}
```
Copy link
Member

Choose a reason for hiding this comment

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

Nit: move $hsm.OriginalManagedHsm.Properties.NetworkAcls into the ```powershell block because the idea is to split the input and output so that user can simply copy paste the input and easily replicate the example

This applies to multiple examples in the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants