Skip to content
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

Replace Microsoft.Azure.Storage.Blob with Azure.Storage.Blobs package #191

Merged
merged 6 commits into from
Apr 20, 2024

Conversation

inthemedium
Copy link
Contributor

@inthemedium inthemedium commented Apr 18, 2024

Replace Microsoft.Azure.Storage.Blob with Azure.Storage.Blobs package.

Upgrading is a good idea because

  1. Microsoft.Azure.Storage.Blob is no longer supported
  2. Azure.Storage.Blobs will get bug fixes/new features
  3. Azure.Storage.Blobs supports modern auth (service principals, managed identities)

@emgarten
Copy link
Owner

I'll update the github actions so we can run the full functional tests against this branch. The functional tests will run against a real storage account and verify things are working.

@inthemedium inthemedium changed the title Dev/jetyson/azure storage Replace Microsoft.Azure.Storage.Blob with Azure.Storage.Blobs package Apr 19, 2024
return new AzureFileSystemLock(blob, messageBlob, log);
}

public override async Task<IReadOnlyList<ISleetFile>> GetFiles(ILogger log, CancellationToken token)
{
string prefix = null;
var useFlatBlobListing = true;
var blobListingDetails = BlobListingDetails.All;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't an 1:1 equivalent of this enum in the new API, so I kept the default values which looks like this:

        /// <summary>
        /// The <see cref="GetBlobsAsync"/> operation returns an async
        /// sequence of blobs in this container.  Enumerating the blobs may
        /// make multiple requests to the service while fetching all the
        /// values.  Blobs are ordered lexicographically by name.
        ///
        /// For more information, see
        /// <see href="https://docs.microsoft.com/rest/api/storageservices/list-blobs">
        /// List Blobs</see>.
        /// </summary>
        /// <param name="traits">
        /// Specifies trait options for shaping the blobs.
        /// </param>
        /// <param name="states">
        /// Specifies state options for filtering the blobs.
        /// </param>
        /// <param name="prefix">
        /// Specifies a string that filters the results to return only blobs
        /// whose name begins with the specified <paramref name="prefix"/>.
        /// </param>
        /// <param name="cancellationToken">
        /// Optional <see cref="CancellationToken"/> to propagate
        /// notifications that the operation should be cancelled.
        /// </param>
        /// <returns>
        /// An <see cref="AsyncPageable{T}"/> describing the
        /// blobs in the container.
        /// </returns>
        /// <remarks>
        /// A <see cref="RequestFailedException"/> will be thrown if
        /// a failure occurs.
        /// </remarks>
        public virtual AsyncPageable<BlobItem> GetBlobsAsync(
            BlobTraits traits = BlobTraits.None,
            BlobStates states = BlobStates.None,
            string prefix = default,
            CancellationToken cancellationToken = default)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For states the default looks correct.
Possibly the metadata trait is needed to get the properties such as the content type and content encoding, I can't tell from the docs if those come down by default.

@inthemedium inthemedium marked this pull request as ready for review April 19, 2024 19:28
@inthemedium
Copy link
Contributor Author

I'll update the github actions so we can run the full functional tests against this branch. The functional tests will run against a real storage account and verify things are working.

Additionally, I ran the tests against a real account and ran every command against a real account with the debugger attached.

@emgarten
Copy link
Owner

For some reason the linux builds are unhappy

Error: /home/runner/work/Sleet/Sleet/src/SleetLib/FileSystem/AzureFile.cs(66,33): error CS0111: Type 'AzureFile' already defines a member called 'CopyFromSource' with the same parameter types [/home/runner/work/Sleet/Sleet/src/SleetLib/SleetLib.csproj::TargetFramework=netstandard2.0]

@emgarten
Copy link
Owner

Functional tests are passing, things look good except for that AzureFile error on linux which I don't see how that would happen.

@emgarten
Copy link
Owner

I can see now, dotnet format is having a lot of trouble with these changes. This repo is already on the latest version of the tool. I don't see what would cause this unless the new library is bringing something in.

What is happening is that it is creating a bunch of stub not implemented methods in AzureFile. No idea why a tool that is suppose to only be formatting the code would generate all of that, I think it is triggering something else or an analyzer.

I'll make a PR to remove dotnet format.

Copy link
Owner

@emgarten emgarten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed dotnet format and confirmed this branch passes all functional tests on linux with that change.

I did some testing locally and everything is working well for me with Azure. The blob properties are showing up correctly so it looks like the default settings work fine.

Thanks for the work on this! 🏆

@emgarten emgarten merged commit 433a0db into emgarten:main Apr 20, 2024
5 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants