-
Notifications
You must be signed in to change notification settings - Fork 4
Bugfix for error when creating a new Azure Keyvault #71
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
Conversation
joevanwanzeeleKF
commented
Nov 6, 2025
- bug fix for error when creating new Azure Keyvaults
- documentation updates
Release 3.1.8
Release 3.1.9
…ead property. Additional documentation.
…factor/azurekeyvault-orchestrator into create_vault_bugfix_ab#78506
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.
Pull Request Overview
This PR fixes a bug that occurred when creating new Azure Keyvaults by making the custom alias required instead of optional, which resolves errors related to invalid certificate names in Azure Keyvault. The changes also include documentation updates to clarify alias naming requirements.
Key Changes:
- Changed
CustomAliasAllowedfrom "Optional" to "Required" to enforce proper certificate naming in Azure Keyvault - Added documentation warning about alias naming restrictions (alphanumeric and hyphens only)
- Improved null safety in job property parsing to prevent errors when creating new vaults
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Updated CustomAliasAllowed to "Required" to enforce alias usage |
| docsource/content.md | Fixed anchor link reference |
| docsource/akv.md | Updated terminology and added warning about alias naming requirements |
| README.md | Updated terminology, fixed anchor link, updated CustomAliasAllowed documentation, and added alias naming warning |
| CHANGELOG.md | Added version 3.1.11 release notes |
| AzureKeyVault/Jobs/Management.cs | Improved null safety by adding defensive checks for JobProperties before accessing dictionary values |
| AzureKeyVault/Jobs/AzureKeyVaultJob.cs | Changed null check to string.IsNullOrEmpty for better validation |
| AzureKeyVault/AzureClient.cs | Refactored resource group retrieval, added debug logging, and enhanced error handling with specific exception types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.LogTrace("calling getAsync on resourcegroup..."); | ||
| await resourceGroup.GetAsync(); | ||
| logger.LogTrace("completed getAsync on resource group..."); | ||
|
|
||
| var s = resourceGroup.HasData.ToString(); | ||
|
|
||
| logger.LogTrace($"resource group has data?: {s}"); | ||
|
|
Copilot
AI
Nov 11, 2025
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.
This appears to be debug code that should be removed. The variable s is created but never used, and the logging statements around GetAsync() seem excessive for production code. Consider removing lines 165-171 or clarifying if this debug logging is intentional.
| logger.LogTrace("calling getAsync on resourcegroup..."); | |
| await resourceGroup.GetAsync(); | |
| logger.LogTrace("completed getAsync on resource group..."); | |
| var s = resourceGroup.HasData.ToString(); | |
| logger.LogTrace($"resource group has data?: {s}"); |
| var resourceGroups = subscription.GetResourceGroups(); | ||
| ResourceGroupResource resourceGroup = await resourceGroups.GetAsync(VaultProperties.ResourceGroupName); | ||
| logger.LogTrace("calling getAsync on resourcegroup..."); | ||
| await resourceGroup.GetAsync(); |
Copilot
AI
Nov 11, 2025
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.
Unnecessary API call: await resourceGroup.GetAsync() is called but its result is not used. The previous line already calls GetAsync() to retrieve the resource group. This redundant call should be removed to avoid unnecessary network overhead.
| await resourceGroup.GetAsync(); |
| - 3.1.11 | ||
| - bug fix for error when creating new Azure Keyvaults | ||
| - documentation updates | ||
|
|
Copilot
AI
Nov 11, 2025
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.
Inconsistent whitespace: this line uses spaces for indentation while the rest of the file likely uses tabs (based on the CHANGELOG file structure). This should use tabs to be consistent with the formatting of line 5.
|
|
||
| > :warning: | ||
| > The alias you provide when enrolling a certificate will be used as the certificate name in Azure Keyvault. | ||
| > Consequently; [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). |
Copilot
AI
Nov 11, 2025
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.
Inconsistent punctuation: A semicolon is used here instead of a comma. Change "Consequently;" to "Consequently," for proper grammar.
| > Consequently; [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). | |
| > Consequently, [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). |
|
|
||
| > :warning: | ||
| > The alias you provide when enrolling a certificate will be used as the certificate name in Azure Keyvault. | ||
| > Consequently; [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). |
Copilot
AI
Nov 11, 2025
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.
Inconsistent punctuation: A semicolon is used here instead of a comma. Change "Consequently;" to "Consequently," for proper grammar.
| > Consequently; [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). | |
| > Consequently, [it must _only_ contain alphanumeric characters and hyphens](https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault). |
| config.JobProperties.TryGetValue(EntryParameters.TAGS, out object tagsJSONObj); | ||
| config.JobProperties.TryGetValue(EntryParameters.PRESERVE_TAGS, out object preserveTagsObj); | ||
| tagsJSON = tagsJSONObj == null ? string.Empty : tagsJSONObj.ToString(); | ||
| preserveTags = preserveTagsObj == null ? false : Boolean.Parse(preserveTagsObj.ToString()); |
Copilot
AI
Nov 11, 2025
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.
The expression 'A ? false : B' can be simplified to '!A && B'.
| preserveTags = preserveTagsObj == null ? false : Boolean.Parse(preserveTagsObj.ToString()); | |
| preserveTags = preserveTagsObj != null && Boolean.Parse(preserveTagsObj.ToString()); |
…b#78506 # Conflicts: # CHANGELOG.md # README.md
ddf79c1 to
080ccce
Compare
Merge 3.1.10 to main
…factor/azurekeyvault-orchestrator into create_vault_bugfix_ab#78506