AMLFS Import Create/Cancel/Get/Delete commands #1492
Conversation
rebecca-makar
requested review from
a team and
wolfgang-desalvador
as code owners
rebecca-makar
requested review from
JonathanCrd,
conniey,
hallipr,
msalaman,
vukelich,
weshaggard and
xiangyan99
| - job-name: Name for the import job (auto-generated if not provided) | ||
| - conflict-resolution-mode: How to handle conflicting files (Fail, Skip, OverwriteIfDirty, OverwriteAlways) | ||
| - import-prefixes: Blob prefixes to import (comma-separated, imports all if not specified) | ||
| - maximum-errors: Maximum errors allowed before job failure (-1: infinite, 0: immediate exit, default: none) |
There was a problem hiding this comment.
Are none and 0 the same thing here? Not knowing the REST API they seem the same to me, if not might be worth adding more description here.
Or, am I reading this wrong where the default is 0 but we decided to state none?
| - filesystem-name: The name of the AMLFS filesystem | ||
| Optional options: | ||
| - job-name: Name for the import job (auto-generated if not provided) | ||
| - conflict-resolution-mode: How to handle conflicting files (Fail, Skip, OverwriteIfDirty, OverwriteAlways) |
There was a problem hiding this comment.
What's the default here if not specified?
| public static readonly Option<bool> WaitForCompletionOption = new( | ||
| "--wait-for-completion" | ||
| ) | ||
| { | ||
| Required = false, | ||
| Description = "Wait for the import operation to complete before returning. Default: false (return immediately after starting import)." | ||
| }; | ||
|
|
||
| public static readonly Option<int> TimeoutMinutesOption = new( | ||
| "--timeout-minutes" | ||
| ) | ||
| { | ||
| Required = false, | ||
| Description = "Maximum time in minutes to wait for import completion (only used when --wait-for-completion is true). Default: 60 minutes." | ||
| }; |
There was a problem hiding this comment.
I don't believe these are being used
| { | ||
| throw new ArgumentException($"Invalid conflict resolution mode: {conflictResolutionMode}. Valid values: {string.Join(", ", validModes)}", nameof(conflictResolutionMode)); | ||
| } | ||
| // Note: Setting this property will need to be implemented once we find the correct enum type |
There was a problem hiding this comment.
Should this be blocked from merging until this is done?
| catch (RequestFailedException rfe) | ||
| { | ||
| throw new Exception($"Failed to create import job for filesystem '{filesystemName}': {rfe.Message}", rfe); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new Exception($"Failed to create import job for filesystem '{filesystemName}': {ex.Message}", ex); | ||
| } |
There was a problem hiding this comment.
Given these do the same thing, should they just be merged into a single catch block.
| catch (RequestFailedException rfe) when (rfe.Status == 404) | ||
| { | ||
| throw new Exception($"Import job '{jobName}' not found for filesystem '{filesystemName}'", rfe); | ||
| } |
There was a problem hiding this comment.
Do we want to throw this as an error or return this as a success but with a message about the job not existing.
Throwing an error to delete a resource that doesn't exist feels off given the intended outcome still is the same.
| // Get the filesystem | ||
| var fs = await rg.GetAmlFileSystems().GetAsync(filesystemName, cancellationToken: cancellationToken); |
There was a problem hiding this comment.
Seems we use different APIs and have different error handling for retrieval of the filesystem. Is this just an inconsistency that should be fixed or do we want this different behavior?
| // TotalBlobsErrored and TotalRequests may not be available in this SDK version | ||
| TotalBlobsErrored = null, | ||
| TotalRequests = null |
There was a problem hiding this comment.
If this information is unavailable at this time, do we want to have these fields existing? Seems to be a waste for now.
What does this PR do?
AMLFS Import Create/Cancel/Get/Delete commands for mcp[Any additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline