Fix Storage Tools Issues.#2674
Open
g2vinay wants to merge 1 commit into
Open
Conversation
…oolset - ST-01: Fix NullReferenceException in GetTableEndpoint by changing parameter from string? to string and removing null-forgiving operator - ST-03: Add Cold and Premium to AccessTier option description - ST-05: Seal StorageService class per project convention - ST-08: Resolve subscription display names to GUIDs in CreateStorageAccount; skip resolution when input is already a GUID - ST-10: Update Sku option description to list all 14 valid SKUs including V2 variants Tests: add ContainerCreateCommandTests.ExecuteAsync_HandlesStorageAccountNotFound
Contributor
There was a problem hiding this comment.
Pull request overview
Improves Azure Storage tool reliability and user-facing CLI help text by aligning storage account creation behavior with subscription name handling used elsewhere, and by documenting the full set of accepted SKU/access-tier values.
Changes:
- Fix
CreateStorageAccountto resolve subscription display names to subscription GUIDs before constructing the ARM resource ID. - Update
--skuand--access-tieroption descriptions to reflect the full set of supported values. - Add a unit test ensuring
container createreturns a 404-style response when the backing service reportsNotFound.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Storage/tests/Azure.Mcp.Tools.Storage.Tests/Blob/Container/ContainerCreateCommandTests.cs | Adds a NotFound unit test for container creation. |
| tools/Azure.Mcp.Tools.Storage/src/Services/StorageService.cs | Resolves subscription names to GUIDs for storage account creation; tightens table client helper signatures. |
| tools/Azure.Mcp.Tools.Storage/src/Options/StorageOptionDefinitions.cs | Expands CLI option descriptions for SKU and access tier values. |
| tools/Azure.Mcp.Tools.Storage/src/Commands/Blob/Container/ContainerCreateCommand.cs | Adds a clarifying comment for the result record. |
| servers/Azure.Mcp.Server/changelog-entries/1779221789527.yaml | Adds a changelog entry describing the Storage tool fixes. |
Comments suppressed due to low confidence (1)
tools/Azure.Mcp.Tools.Storage/src/Services/StorageService.cs:438
CreateTableServiceClienttakes asubscriptionparameter but does not use it (verified within this method). Since the method is nowprivateand only used internally, consider removing the unused parameter and updating the single call site to reduce confusion about what inputs influence the table endpoint/client creation.
private async Task<TableServiceClient> CreateTableServiceClient(
string account,
string subscription,
string? tenant = null,
RetryPolicyOptions? retryPolicy = null,
CancellationToken cancellationToken = default)
| ILogger<StorageService> logger) | ||
| : BaseAzureResourceService(subscriptionService, tenantService), IStorageService | ||
| { | ||
| private readonly ISubscriptionService _subscriptionService = subscriptionService; |
| public static readonly Option<string> Sku = new($"--{SkuName}") | ||
| { | ||
| Description = "The storage account SKU. Valid values: Standard_LRS, Standard_GRS, Standard_RAGRS, Standard_ZRS, Premium_LRS, Premium_ZRS, Standard_GZRS, Standard_RAGZRS.", | ||
| Description = "The storage account SKU. Valid values: Standard_LRS, Standard_GRS, Standard_RAGRS, Standard_ZRS, " + |
Contributor
There was a problem hiding this comment.
This change is incorrect, instead we should be updating the tool description to indicate we only support StorageV2 accounts. The additional SKUs added here are for different account types.
https://learn.microsoft.com/rest/api/storagerp/srp_sku_types
| return context.Response; | ||
| } | ||
|
|
||
| // Strongly-typed result record |
Comment on lines
+103
to
+107
| // Resolve subscription display name to GUID (consistent with all other storage operations). | ||
| // Skip resolution when subscription is already a GUID to avoid an unnecessary round-trip. | ||
| var subscriptionId = _subscriptionService.IsSubscriptionId(subscription) | ||
| ? subscription | ||
| : (await _subscriptionService.GetSubscription(subscription, tenant, retryPolicy, cancellationToken)).Data.SubscriptionId; |
Contributor
There was a problem hiding this comment.
Do we actually need to resolve to a GUID here?
This file contains hidden or 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
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.
No description provided.