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

Upgrade AzureBlockBlob storage backend to use Azure blob storage library v12 #6580

Merged
merged 7 commits into from
Jan 9, 2021

Conversation

JanusAsmussen
Copy link
Contributor

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

This PR upgrades the AzureBlockBlob storage backend to use Azure blob storage library v12. This replaces the currently used SDK (Azure blob storage library v2.1) which is considered a legacy version by Microsoft.

@lgtm-com
Copy link

lgtm-com bot commented Jan 6, 2021

This pull request fixes 2 alerts when merging a271577 into 2dd6769 - view on LGTM.com

fixed alerts:

  • 2 for Non-exception in 'except' clause

@auvipy
Copy link
Member

auvipy commented Jan 7, 2021

could you please check if there are any other things that need to be updated? integration test/tox etc?

@lgtm-com
Copy link

lgtm-com bot commented Jan 7, 2021

This pull request introduces 17 alerts and fixes 3 when merging 0493f66 into 2c6f46d - view on LGTM.com

new alerts:

  • 12 for Wrong number of arguments in a call
  • 3 for Module is imported with 'import' and 'import from'
  • 1 for __eq__ not overridden when adding attributes
  • 1 for Use of the return value of a procedure

fixed alerts:

  • 2 for Non-exception in 'except' clause
  • 1 for Wrong number of arguments in a call

@JanusAsmussen
Copy link
Contributor Author

Hi,

Both unit & integration tests related to the AzureBlockBlob storage backend are passing when using the environment set up with Docker and docker-compose. However, I had to change the azurite docker image used when spinning up the environment (It is now using the official Microsoft image). I've committed this change to the remote branch.

Regarding tox: I do not believe anything should be changed.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you check the following alerts?
This pull request introduces 17 alerts and fixes 3 when merging 0493f66 into 2c6f46d - view on LGTM.com

new alerts:

12 for Wrong number of arguments in a call
3 for Module is imported with 'import' and 'import from'
1 for __eq__ not overridden when adding attributes
1 for Use of the return value of a procedure

@JanusAsmussen
Copy link
Contributor Author

Commit 0493f66 changes a single string in a yaml file. I would believe these warnings are caused by 2c6f46d?

Copy link
Member

@matusvalo matusvalo left a comment

Choose a reason for hiding this comment

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

LGTM for me.

@matusvalo
Copy link
Member

@auvipy do you have still objections?

@auvipy auvipy added this to the 5.0.6 milestone Jan 9, 2021
@auvipy auvipy merged commit f6ca745 into celery:master Jan 9, 2021
@thedrow thedrow modified the milestones: 5.0.6, 5.1.0 Feb 24, 2021
@thedrow thedrow added this to Done in Celery 5.1.0 Feb 24, 2021
jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
…ary v12 (celery#6580)

* Upgrade AzureBlockBlob backend to use library azure-storage-blob v12

* Fix minor bug in AzureBlockBlob backend unit test

* Upgrade AzureBlockBlob backend to use library azure-storage-blob v12

* Fix minor bug in AzureBlockBlob backend unit test

* Bug fixes in AzureBlockBlob class and unit tests

* Update docker-compose.yml to use Microsoft's official azurite docker image

Co-authored-by: Janus Asmussen <jjasmussen@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Celery 5.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants