[KeyVault] Handle Nullable Parameters for Certificate Auto-Renewal in Set-AzKeyVaultCertificatePolicy #25844
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
️✔️Az.Accounts
️✔️Az.Compute
️✔️Az.CosmosDB
️✔️Az.EventHub
|
| Type | Cmdlet | Description | Remediation |
|---|---|---|---|
| Get-AzKeyVaultManagedHsmRegion | Get-AzKeyVaultManagedHsmRegion Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. | Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue | |
| Get-AzKeyVaultManagedHsmRegion | Get-AzKeyVaultManagedHsmRegion changes the confirm impact. Please ensure that the change in ConfirmImpact is justified | Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact. |
⚠️ Windows PowerShell - Windows
| Type | Cmdlet | Description | Remediation |
|---|---|---|---|
| Get-AzKeyVaultManagedHsmRegion | Get-AzKeyVaultManagedHsmRegion Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. | Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue | |
| Get-AzKeyVaultManagedHsmRegion | Get-AzKeyVaultManagedHsmRegion changes the confirm impact. Please ensure that the change in ConfirmImpact is justified | Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact. |
️✔️Help Example Check
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Help File Existence Check
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️File Change Check
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️UX Metadata Check
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
⚠️ Test
⚠️ - Linux
Type Title Current Coverage Description ⚠️ Test Coverage Less Than 50% 22.09 % Test coverage for the module cannot be lower than 50%.
⚠️ - MacOS
Type Title Current Coverage Description ⚠️ Test Coverage Less Than 50% 22.09% Test coverage for the module cannot be lower than 50%.
⚠️ PowerShell Core - Windows
Type Title Current Coverage Description ⚠️ Test Coverage Less Than 50% 22.09% Test coverage for the module cannot be lower than 50%.
⚠️ Windows PowerShell - Windows
Type Title Current Coverage Description ⚠️ Test Coverage Less Than 50% 22.09% Test coverage for the module cannot be lower than 50%.
️✔️Az.ManagedServiceIdentity
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.Monitor
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.Network
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Test
️✔️ - Linux
️✔️ - MacOS
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.OperationalInsights
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.PrivateDns
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.Resources
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.ServiceBus
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Test
️✔️ - Linux
️✔️ - MacOS
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.Sql
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Test
️✔️ - Linux
️✔️ - MacOS
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
️✔️Az.Storage
️✔️Build
️✔️PowerShell Core - Windows
️✔️Windows PowerShell - Windows
BethanyZhou
requested changes
Aug 19, 2024
There was a problem hiding this comment.
check following issues reported by our CIs. If it's false positive, suppress them by https://github.com/Azure/azure-powershell/blob/main/documentation/Debugging-StaticAnalysis-Errors.md#breaking-changes
D:\a_work\1\s\artifacts/StaticAnalysisResults\BreakingChangeIssues.csv Errors
"Module","ClassName","Target","Severity","ProblemId","Description","Remediation"
"Az.KeyVault","Microsoft.Azure.Commands.KeyVault.SetAzureKeyVaultCertificatePolicy","Set-AzKeyVaultCertificatePolicy","0","1050","The parameter set '__AllParameterSets' for cmdlet 'Set-AzKeyVaultCertificatePolicy' has been removed.","Add parameter set '__AllParameterSets' back to cmdlet 'Set-AzKeyVaultCertificatePolicy'."
"Az.KeyVault","Microsoft.Azure.Commands.KeyVault.SetAzureKeyVaultCertificatePolicy","Set-AzKeyVaultCertificatePolicy","0","1050","The parameter set 'ByValue' for cmdlet 'Set-AzKeyVaultCertificatePolicy' has been removed.","Add parameter set 'ByValue' back to cmdlet 'Set-AzKeyVaultCertificatePolicy'."
"Az.KeyVault","Microsoft.Azure.Commands.KeyVault.SetAzureKeyVaultCertificatePolicy","Set-AzKeyVaultCertificatePolicy","0","1050","The parameter set 'ExpandedRenewNumber' for cmdlet 'Set-AzKeyVaultCertificatePolicy' has been removed.","Add parameter set 'ExpandedRenewNumber' back to cmdlet 'Set-AzKeyVaultCertificatePolicy'."
"Az.KeyVault","Microsoft.Azure.Commands.KeyVault.SetAzureKeyVaultCertificatePolicy","Set-AzKeyVaultCertificatePolicy","0","1050","The parameter set 'ExpandedRenewPercentage' for cmdlet 'Set-AzKeyVaultCertificatePolicy' has been removed.","Add parameter set 'ExpandedRenewPercentage' back to cmdlet 'Set-AzKeyVaultCertificatePolicy'."
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Show resolved
Hide resolved
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Outdated
Show resolved
Hide resolved
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Outdated
Show resolved
Hide resolved
…hhh/azure-powershell into fix/28957664-disable-auto-renewal
|
This PR was labeled "needs-revision" because it has unresolved review comments or CI failures. |
BethanyZhou
requested changes
Sep 2, 2024
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Outdated
Show resolved
Hide resolved
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Outdated
Show resolved
Hide resolved
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Show resolved
Hide resolved
BethanyZhou
reviewed
Sep 2, 2024
src/KeyVault/KeyVault/Commands/Certificate/Policy/SetAzureKeyVaultCertificatePolicy.cs
Outdated
Show resolved
Hide resolved
BethanyZhou
changed the title
github-actions bot
pushed a commit
that referenced
this pull request
Sep 9, 2024
… Set-AzKeyVaultCertificatePolicy (#25844) * Added manual validation for piped attributes * Updated ChangeLog * Validate using a helper function * Added back ValueFromPipeline Testing * Removed ValueFromPipelineProperty * Added Suggested Changes * Dummy Commit * Added Pester Testing * Added CI error suppression * Updated Help Docs for cmdlet --------- Co-authored-by: Yash Patil <[email protected]>
6 tasks
VeryEarly
added a commit
that referenced
this pull request
Sep 9, 2024
… Set-AzKeyVaultCertificatePolicy (#25844) (#26055) * Added manual validation for piped attributes * Updated ChangeLog * Validate using a helper function * Added back ValueFromPipeline Testing * Removed ValueFromPipelineProperty * Added Suggested Changes * Dummy Commit * Added Pester Testing * Added CI error suppression * Updated Help Docs for cmdlet --------- Co-authored-by: Yash <[email protected]> Co-authored-by: Yash Patil <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses issue #25649, where users were unable to disable auto-renewal on a KeyVault certificate policy. The changes include:
Manual Validation for Nullable Parameters: The parameters
RenewAtNumberOfDaysBeforeExpiryandRenewAtPercentageLifetimewere originally set with ValidateRange attributes, which caused issues when trying to set them to null. The validation logic has been moved to theExecuteCmdletmethod to manually validate these parameters only when they are not null.Updated Command Behavior: Adjusted the behavior of
Set-AzKeyVaultCertificatePolicyto allow for proper handling of nullable parameters, ensuring that users can disable auto-renewal by setting these parameters to null.Improved Pipeline Handling: Addressed an issue where the
Curve,KeySize, andCertificateTransparencyproperties were incorrectly being validated from the pipeline due toValueFromPipelineByPropertyName. These validations have been adjusted to prevent incorrect mappings and allow the command to function as expected.Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:Additional Notes:
This PR improves the usability of the Set-AzKeyVaultCertificatePolicy cmdlet and ensures that it behaves as expected when managing certificate policies within Azure KeyVault.
ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.