Fix pidbox not using default channels on 4.x branch #1256
Conversation
|
The second commit updates the dev requirements because the master for vine and py-amqp have dropped support for Python 2.7 and 3.5 while the Kombu 4.x branch continues to need to support those versions. |
lambacck
force-pushed
the
fix-pidbox-not-used-default-channels
branch
3 times, most recently
from
5cfd714
to
e419d5c
Compare
|
I rearranged commits to show tests going from green to red. I'm feeding the commits one at a time because that's what's required to get test runs to fire. Travis cancels a test run if I push a new commit so I'm waiting for the previous run to complete before I push the next commit. |
lambacck
force-pushed
the
fix-pidbox-not-used-default-channels
branch
2 times, most recently
from
3047dc2
to
171b6a4
Compare
|
The test failure in |
lambacck
force-pushed
the
fix-pidbox-not-used-default-channels
branch
from
171b6a4
to
4598f14
Compare
|
Hi @lambacck. Thank you for your PR. I think to accept your PR, you need to create it against master. |
| @@ -0,0 +1 @@ | |||
| 3.6.11 | |||
There was a problem hiding this comment.
What is purpose of this file? Is it needed in PR?
There was a problem hiding this comment.
nope that's an accidental add because of pyenv.
I'm putting it against 4.x because I'm still using Celery 4 and am not yet moving to Celery 5. I'm happy to also open a PR against master with the equivalent changes. |
lambacck
force-pushed
the
fix-pidbox-not-used-default-channels
branch
from
9e922c3
to
5c08827
Compare
Not all cases are properly using the default channel even when detecting for it. Fixes: celery#1201
|
Okay. I was finally able to get tests running locally on my mac so I think this time tests will be green everywhere but windows because install of brotli isn't working on windows tests. Once I see green here I'll also open a PR agains master with the 2 commits related to solving the issue (i.e. without the 4.x dependency pinning). |
|
Okay, I think this is properly ready for review now. I'll go make the PR against master that's the equivalent. |
lambacck
changed the title
| @@ -1,2 +1,2 @@ | |||
| https://github.com/celery/py-amqp/zipball/master | |||
| https://github.com/celery/vine/zipball/master | |||
| https://github.com/celery/py-amqp/zipball/v2.6 | |||
There was a problem hiding this comment.
is it really need to be changed?
There was a problem hiding this comment.
master for those projects has dropped support for Python 2.7 and Python 3.5. This pins the dev-dependencies for the 4.x branch here to the branches in those projects so that kombu 4 (and thus Celery 4) can maintain support for python 2.7 and 3.5 in the spirit of the LTS branch declared with the release of Celery 5.
There was a problem hiding this comment.
Related, it seems that brotli on windows needs some kind of pinning to make it continue to work for python 2.7 but I didn't want to dig too hard into what that was. That's why appveyor tests are failing.
There was a problem hiding this comment.
Seems like it will work with 1.0.7 based on this: google/brotli#848
|
Closing in favor of #1258. To backport this functionality to 4.x, I prefer to cherrypick commit from master. |
|
That's what this branch is. The master changes cherry-picked. But also the 4.x branch won't pass tests until you do the version pinning things I did in the first commit (which aren't on master). |
Not all cases are properly using the default channel even when detecting
for it.
This is directed at the 4.x branch because I'm using celery 4.4.7 and trying to see if this change fixes #1063 for me and I'm not ready to go to Celery 5.
Fixes: #1201