Removing blob storage SDK leakage into the code base #9981
Conversation
Directory.Build.props
Outdated
| @@ -2,7 +2,7 @@ | |||
| <PropertyGroup> | |||
| <NuGetClientPackageVersion>6.9.1</NuGetClientPackageVersion> | |||
| <ServerCommonPackageVersion>2.120.0</ServerCommonPackageVersion> | |||
| <NuGetJobsPackageVersion>4.3.0-main-9408418</NuGetJobsPackageVersion> | |||
| <NuGetJobsPackageVersion>4.3.0-agr-azst-9643015</NuGetJobsPackageVersion> | |||
There was a problem hiding this comment.
This would have to be updated with a separate PR as NuGet.Jobs release depends on this PR.
There was a problem hiding this comment.
This seems circular. Should we get away from this dependency? (sometime in the future)
There was a problem hiding this comment.
Sometimes this circular dependency makes hard to reason with runtime issue caused by dependency injection. Not sure exactly what was the actual dll consumed.
There was a problem hiding this comment.
We have jobs in Gallery repo that use base classes from NuGet.Jobs.
There was a problem hiding this comment.
I figured it would be something like that--I'll create a task for moving them, although that would also involve nugetizing some additional gallery dependencies to support them, I expect. DI, for one may make this a pipe dream.
| { | ||
| return await @delegate(); | ||
| } | ||
| catch (StorageException ex) when (ex.RequestInformation?.ExtendedErrorInformation?.ErrorCode == BlobErrorCodeStrings.ContainerNotFound) |
There was a problem hiding this comment.
To avoid code duplication, consider creating a single method to handle the exceptions, and call this method from both WrapStorageException and WrapStorageException.
There was a problem hiding this comment.
Do you have a change in mind? I wasn't able to come up with a solution that would work for both cases.
Shorter HttpStatusCode checks.
|
Could please add tracking issue into desc? |
@erdembayar Done. |
|
|
||
| namespace NuGetGallery | ||
| { | ||
| public enum CloudBlobLocationMode |
There was a problem hiding this comment.
It looks this one is copy of https://learn.microsoft.com/en-us/dotnet/api/microsoft.azure.storage.retrypolicies.locationmode?view=azure-dotnet-legacy.
Should we add a comment above so the next person who is reading the code knows where those enum values are coming from?
Addresses: https://github.com/NuGet/Engineering/issues/5456
We have wrappers around blob storage SDK (
CloudBlobWrapper,CloudBlobContainerWrapper,CloudBlobClientWrapper), unfortunately, they leak underlying SDK types as function arguments, return types and exceptions thrown, which defeats the purpose and complicates swapping the underlying implementation.This change aims to stop that leakage. Introduced more wrappers around concepts that we use (
BlobListContinuationToken,CloudBlobCopyState,CloudBlobCopyStatus,CloudBlobLocationMode,CloudBlobPropertiesWrapper,ListingDetails), propagated other wrappers that we had, but didn't use everywhere (AccessConditionWrapper,FileUriPermissions), wrappedStorageExceptionwith our own exception hierarchy following the checks existing in the code, simplifying catch expressions.