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.

Sign up for GitHub

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

Jump to bottom

Fix Bugs in new v12 Azure Storage #868

Merged
merged 24 commits into from
Apr 13, 2023
Merged

Fix Bugs in new v12 Azure Storage #868

merged 24 commits into from
Apr 13, 2023

Conversation

wsugarman
Copy link
Contributor

@wsugarman wsugarman commented Feb 20, 2023 •
edited
Loading

A few tests failed when updating Durable Task in the Durable Functions Extension. Includes the following changes:

  • Merge with main
  • Escape blob URLs generated by MessageManager
  • Refactor storage account service client constructors
  • Use new TrackingServiceClientProvider for the property of the same name in the orchestration service settings given that both blob and table clients are necessary
    • StorageAccountClientProvider now derives from this new type
  • Quality-of-life: change URL path separator for blobs
  • Align storage package versions with that of the Azure Functions host
    • This includes using a version of the TableClient without a Uri property, and so it must be computed using reflection

I made the following changes (plus tests). I also bumped the preview version:

  • /TrackingServiceClientProvider.cs
  • /Net/UriPath.cs
  • /Storage/Blob.cs
  • /Storage/BlobContainer.cs
  • /Storage/Queue.cs
  • /Storage/Table.cs
  • /Storage/TableClientExtensions.cs

wsugarman
wsugarman commented Feb 20, 2023
View reviewed changes
src/DurableTask.AzureStorage/Storage/Blob.cs
throw new ArgumentException("Missing blob container", nameof(blobUri));
}

var builder = new BlobUriBuilder(blobUri);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw the library contains this very helpful class

cgillum
cgillum reviewed Feb 21, 2023
View reviewed changes
Copy link
Collaborator

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

These changes look good to me. Just a couple tiny readability comments. Let me know if you need help debugging the failed entity test(s) that you mentioned in the PR description.

src/DurableTask.AzureStorage/Net/UriPath.cs Show resolved Hide resolved
Test/DurableTask.AzureStorage.Tests/MessageManagerTests.cs Outdated Show resolved Hide resolved
@wsugarman wsugarman marked this pull request as ready for review March 2, 2023 16:44
wsugarman and others added 13 commits March 2, 2023 08:56
@cgillum
Copy link
Collaborator

cgillum commented Apr 13, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@cgillum
Copy link
Collaborator

cgillum commented Apr 13, 2023

Looks like our CI pipeline isn't set up to properly work with PRs to feature branches. :-/

@wsugarman sorry that this PR fell off my radar. Do you feel that it's ready to be merged into the feature branch? I quickly reviewed the latest changes and it looks like they were mostly updates to keep parity with recent changes in main? I'd be happy to merge this if you think it's good to go.

@wsugarman
Copy link
Contributor Author

Looks like our CI pipeline isn't set up to properly work with PRs to feature branches. :-/

@wsugarman sorry that this PR fell off my radar. Do you feel that it's ready to be merged into the feature branch? I quickly reviewed the latest changes and it looks like they were mostly updates to keep parity with recent changes in main? I'd be happy to merge this if you think it's good to go.

@cgillum No problem! I've run the tests for the AzureStorage project locally, and everything passes. It should be good to merge into the feature branch.

@cgillum cgillum merged commit 4cefd74 into Azure:azure-storage-v12 Apr 13, 2023
@wsugarman wsugarman deleted the azure-storage-v12/fixes branch May 30, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgillum cgillum cgillum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@wsugarman @cgillum @amdeel @nytian @NeetArt @sebastianburckhardt @bachuv @davidmrdavid @madevoge