-
Notifications
You must be signed in to change notification settings - Fork 27
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
♻️ Completely remove celery #2730
♻️ Completely remove celery #2730
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2730 +/- ##
========================================
- Coverage 78.1% 77.2% -1.0%
========================================
Files 647 640 -7
Lines 26657 26460 -197
Branches 2581 2560 -21
========================================
- Hits 20841 20444 -397
- Misses 5128 5343 +215
+ Partials 688 673 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
af40522
to
c16309e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 long live to celery! Probably will see her back soon
Other changes (not in the description)
- adds
settings_library
to webserver's requirements - using
settings_lbirary.RabbitSettings
as computation settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few library upgrades slipped. Please remove them before merging.
@@ -61,26 +49,10 @@ chardet==4.0.0 | |||
# aiohttp | |||
charset-normalizer==2.0.8 | |||
# via requests | |||
click==7.1.2 | |||
click==8.0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this slipped through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be due to the fact that celery was blocking it before? @pcrespov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would also be my guess but I'd do all the upgrades separately in different PRs, as usual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, no this is a new requirement coming with settings-library so I think that is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case is justified (since it was produce with a target upgrade) and there is no need to create a separate PR for it, provided all tests pass
@@ -12,9 +12,9 @@ bump2version==1.0.1 | |||
# via -r requirements/../../../../requirements/devenv.txt | |||
cfgv==3.3.1 | |||
# via pre-commit | |||
click==7.1.2 | |||
click==8.0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another click upgrade
She? Now you confuse me. Hardware is female and software is male, no? |
What do these changes do?
Well, celery is completely removed.
⚠️ devops: remove any entry with Flower in Ops.
Also as celery is gone, there is no need for the Flower application anymore. Thus:
Related issue/s
How to test
Checklist