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

[WIP] Fix redbeat doesn't work with RC release issue #89

Conversation

fangpenlin
Copy link

@fangpenlin fangpenlin commented Mar 31, 2018

Fix #88

Work in progress


This change is Reviewable

@@ -32,6 +32,7 @@
install_requires=[
'redis',
'celery',
'packaging',
Copy link
Owner

Choose a reason for hiding this comment

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

I can't recall is packaging included by default in newer python releases? I'm -0 on adding a dependency for such a minimal "feature". Would it be possible to sniff the version of Celery by doing something like

"""
CELERY_4_OR_GREATER = True
try:
import SomethingThatOnlyExistsInCelery4
except ImportError:
CELERY_4_OR_GREATER = False
"""

I'm not sure that is any better but would like to hear other's thoughts.

Choose a reason for hiding this comment

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

Coming in from outside, I've encountered this issue. You don't need to add packaging as a dependency at all.

You should probably use from pkg_resources import parse_version as pkg_resources is part of setuptools and will (almost) always be provided in the field.

Copy link

Choose a reason for hiding this comment

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

Either way, this definitely an issue. Would be great to accept this PR vs. no fix at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

celery.VERSION is a tuple of ints that can be be used for comparisons without needing to install packaging. packaging is not installed by default, but might be there anyway because of newer pip releases or other packages using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fangpenlin the @lambacck solution seems to be the simplest one:

diff --git a/redbeat/schedulers.py b/redbeat/schedulers.py
index a2e0ffe..06f0746 100644
--- a/redbeat/schedulers.py
+++ b/redbeat/schedulers.py
@@ -15,7 +15,7 @@ try:
 except ImportError:
     import json

-from celery import __version__ as celery_version
+from celery import VERSION as CELERY_VERSION
 from celery.beat import Scheduler, ScheduleEntry, DEFAULT_MAX_INTERVAL
 from celery.utils.log import get_logger
 from celery.signals import beat_init
@@ -36,7 +36,7 @@ from .decoder import (
     from_timestamp, to_timestamp
     )

-CELERY_4_OR_GREATER = StrictVersion(celery_version) >= StrictVersion('4.0')
+CELERY_4_OR_GREATER = CELERY_VERSION[0] >= 4


 def ensure_conf(app):

az-wegift added a commit to wegift/redbeat that referenced this pull request Aug 13, 2018
@sibson
Copy link
Owner

sibson commented Aug 13, 2018

@az-wegift if you want to push this change or create a new PR I can merge it.

@az-wegift
Copy link
Contributor

@sibson Please see #96.

@sibson
Copy link
Owner

sibson commented Aug 14, 2018

closed in favour of #96

@sibson sibson closed this Aug 14, 2018
jezdez pushed a commit to mozilla/telemetry-analysis-service that referenced this pull request Jan 21, 2019
Bumps [celery-redbeat](https://github.com/sibson/redbeat) from 0.11.1 to 0.12.0.
<details>
<summary>Changelog</summary>

*Sourced from [celery-redbeat's changelog](https://github.com/sibson/redbeat/blob/master/CHANGES.txt).*

> 0.12.0 (2018-12-06)
> --------------------
>   - better Celery 4 support, thanks [**az-wegift**](https://github.com/az-wegift)
>   - configurably, automatically reconnect to Redis on timeout error rather than terminate, thanks [**az-wegift**](https://github.com/az-wegift)
>   - password support for Redis Sentinal connections, thanks [**az-wegift**](https://github.com/az-wegift)
>   - bugfix, rrule serialization under Celery 3, thanks [**noamkush**](https://github.com/noamkush)
</details>
<details>
<summary>Commits</summary>

- [`9831499`](sibson/redbeat@9831499) bump version to 0.12.0
- [`80758c2`](sibson/redbeat@80758c2) Merge pull request [#104](https://github-redirect.dependabot.com/sibson/redbeat/issues/104) from noamkush/redis_version
- [`b76a67c`](sibson/redbeat@b76a67c) Added version specifier for redis-py.
- [`97223a6`](sibson/redbeat@97223a6) Merge pull request [#80](https://github-redirect.dependabot.com/sibson/redbeat/issues/80) from wegift/retrying
- [`90cfbe9`](sibson/redbeat@90cfbe9) Reconnect to Redis on connection or time-out error
- [`8635a28`](sibson/redbeat@8635a28) Merge pull request [#96](https://github-redirect.dependabot.com/sibson/redbeat/issues/96) from wegift/celery-version
- [`7c69f18`](sibson/redbeat@7c69f18) Fix interpreting Celery version (sibson/redbeat#89)
- [`3c5c6e2`](sibson/redbeat@3c5c6e2) Add .pytest_cache to .gitignore
- [`7a4c254`](sibson/redbeat@7a4c254) Fix celery4 unit tests
- [`76d681c`](sibson/redbeat@76d681c) more 3.x version bumps
- See full diff in [compare view](sibson/redbeat@v0.11.1...v0.12.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=celery-redbeat&package-manager=pip&previous-version=0.11.1&new-version=0.12.0)](https://dependabot.com/compatibility-score.html?dependency-name=celery-redbeat&package-manager=pip&previous-version=0.11.1&new-version=0.12.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants