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

Migrate to azure-storage-blob-go #9577

Merged
merged 3 commits into from
Oct 5, 2020
Merged

Migrate to azure-storage-blob-go #9577

merged 3 commits into from
Oct 5, 2020

Conversation

elsesiy
Copy link
Contributor

@elsesiy elsesiy commented Jul 23, 2020

The azure sdk for go is maintenance-only for storage, see https://github.com/Azure/azure-sdk-for-go/tree/master/storage\#azure-storage-sdk-for-go-preview
Migrate to new azure-storage-blob-go SDK
Minor test improvements

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 23, 2020

CLA assistant check
All committers have signed the CLA.

@elsesiy elsesiy marked this pull request as ready for review July 23, 2020 07:13
@elsesiy
Copy link
Contributor Author

elsesiy commented Aug 30, 2020

@briankassouf or @kalafut Can you please have a look? I left two TODOs open for discussion but would like to get some momentum going on this, thanks!

@calvn calvn self-requested a review September 2, 2020 21:24
@calvn calvn added this to the 1.6 milestone Sep 2, 2020
@elsesiy
Copy link
Contributor Author

elsesiy commented Sep 21, 2020

@calvn Just rebased again. Can you have a look please?

Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I left a few comments on the PR, mainly around the testing changes and error handling within them. I am still thinking about the proper context and retry values to use.

physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Outdated Show resolved Hide resolved
physical/azure/azure_test.go Show resolved Hide resolved
physical/azure/azure.go Outdated Show resolved Hide resolved
@elsesiy elsesiy requested a review from calvn September 28, 2020 00:29
physical/azure/azure.go Outdated Show resolved Hide resolved
physical/azure/azure.go Outdated Show resolved Hide resolved
@elsesiy elsesiy requested a review from calvn October 1, 2020 16:41
The azure sdk for go is maintenance-only for storage, see https://github.com/Azure/azure-sdk-for-go/tree/master/storage\#azure-storage-sdk-for-go-preview
Migrate to new azure-storage-blob-go SDK
Minor test improvements

Fix #9661
@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 1, 2020

@calvn I believe all comments have been addressed

Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

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

Ran acceptance tests and they passed. The listing test took a bit since it needs to create 5k+ keys on the storage container for that test case.

=== RUN   TestAzureBackend
--- PASS: TestAzureBackend (1.88s)
=== RUN   TestAzureBackend_ListPaging
--- PASS: TestAzureBackend_ListPaging (235.04s)
PASS

@elsesiy
Copy link
Contributor Author

elsesiy commented Oct 3, 2020

Ran acceptance tests and they passed. The listing test took a bit since it needs to create 5k+ keys on the storage container for that test case.

=== RUN   TestAzureBackend
--- PASS: TestAzureBackend (1.88s)
=== RUN   TestAzureBackend_ListPaging
--- PASS: TestAzureBackend_ListPaging (235.04s)
PASS

That's great!! Can we merge it then?

@calvn calvn merged commit c1c8eb7 into hashicorp:master Oct 5, 2020
@elsesiy elsesiy deleted the azstoragesdk branch October 5, 2020 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants