Sync eng/common directory with azure-sdk-tools for PR 14973#46199
Sync eng/common directory with azure-sdk-tools for PR 14973#46199
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
azure-sdk
added
EngSys
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Syncs eng/common with changes from azure-sdk-tools PR 14973, improving storage account cleanup by adding version-aware deletion for VLW (versioned immutable) blob containers so containers can be deleted successfully.
Changes:
- Adds VLW-container detection during storage account cleanup and routes those containers through specialized cleanup.
- Introduces
Remove-VlwContainerBlobsto delete soft-deleted blobs and non-current versions (including clearing legal hold / immutability policy). - Adds inline documentation explaining why standard blob listing doesn’t unblock container deletion in VLW scenarios.
| if (($container | Get-Member 'BlobContainerProperties') -and $container.BlobContainerProperties.HasImmutableStorageWithVersioning) { | ||
| Remove-VlwContainerBlobs -Container $container -StorageAccountName $Account.StorageAccountName -ResourceGroupName $Account.ResourceGroupName | ||
| } else { |
There was a problem hiding this comment.
Get-Member 'BlobContainerProperties' will emit an error when that member doesn’t exist; in runs where $ErrorActionPreference = 'Stop' (common in eng scripts), this can terminate the cleanup unexpectedly. Use a non-throwing property-existence check (e.g., PSObject properties) or add -ErrorAction SilentlyContinue to Get-Member and ensure the condition remains reliable.
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | ||
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } |
There was a problem hiding this comment.
Set-AzStorageBlobLegalHold commonly does not support a -DisableLegalHold switch (many Az.Storage versions use a dedicated Clear-AzStorageBlobLegalHold cmdlet instead). Because the catch is empty, a parameter-binding failure here would be silently ignored, leaving legal holds in place and preventing deletions. Use the correct cmdlet/parameter set for clearing legal holds (and consider surfacing unexpected failures).
| # Unconditionally clear legal holds and immutability policies. Errors are expected for | ||
| # soft-deleted blobs or blobs that don't have these set. | ||
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | ||
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } | ||
| try { | ||
| $blob | Remove-AzStorageBlob -Force | ||
| } catch { | ||
| # Deleting the current version by version ID returns 403 | ||
| # (OperationNotAllowedOnRootBlob); fall back to base blob deletion. | ||
| try { | ||
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force | ||
| } catch { } | ||
| } |
There was a problem hiding this comment.
Multiple empty catch { } blocks make it very hard to distinguish expected/benign failures (e.g., missing immutability policy) from real problems (auth/permission issues, missing modules/parameters, API regressions). Consider narrowing what you suppress (e.g., only specific known error cases) and emitting at least Write-Verbose/Write-Warning for unexpected exceptions so failures don’t silently leave containers partially undeleted.
| function Remove-VlwContainerBlobs($Container, $StorageAccountName, $ResourceGroupName) { | ||
| Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName" | ||
|
|
||
| for ($round = 0; $round -lt 5; $round++) { |
There was a problem hiding this comment.
The retry bound 5 is a magic number. Consider making it a named constant or a parameter (defaulting to 5) so callers can tune the behavior and future maintainers understand/adjust the limit without editing logic.
| for ($round = 0; $round -lt 5; $round++) { | |
| $maxCleanupPasses = 5 | |
| for ($round = 0; $round -lt $maxCleanupPasses; $round++) { |
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#14973 See eng/common workflow