pagure

#5358 fix: BROKER_URL default that honors REDIS_PORT and REDIS_DB
Merged by ngompa. Opened by wombelix.
wombelix/pagure fix_celery_worker_dont_honor_redis_port_db_config_by_default  into  master

Download 5358.patch

Config Parameter BROKER_URL (https://docs.pagure.org/pagure/configuration.html#broker-url) is used by Celery workers and doesn't honor REDIS_PORT and REDIS_DB by default, it's right now expected that the User overwrite the BROKER_URL parameter if a non-standard port / db is required.

Both parameters are defined in default_config.py, also based on Celery Docs: "all fields after the scheme are optional, and will default to localhost on port 6379, using database 0".

Therefore it's safe and more user friendly to change the default of BROKER_URL to:
"redis://%s:%d/%d" % (pagure_config["REDIS_HOST"], pagure_config["REDIS_PORT"], pagure_config["REDIS_DB"])

Result local unit tests (run-tests-container.py --skip-build --fedora):

tests/test_tasks.py
===================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.13, pytest-6.2.2, py-1.11.0, pluggy-0.13.1
rootdir: /pagure
plugins: forked-1.3.0, xdist-2.2.0
gw0 [6] / gw1 [6] / gw2 [6] / gw3 [6]
......                                                                                                                                                                                  [100%]
====================================================================================== warnings summary =======================================================================================
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
  /pagure/tests/__init__.py:13: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp
-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================================================================ 6 passed, 4 warnings in 2.49s ================================================================================
Summary:
  ALL TESTS PASSED
tests/test_pagure_lib_task_mirror.py
===================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.13, pytest-6.2.2, py-1.11.0, pluggy-0.13.1
rootdir: /pagure
plugins: forked-1.3.0, xdist-2.2.0
gw0 [8] / gw1 [8] / gw2 [8] / gw3 [8]
........                                                                                                                                                                                [100%]
====================================================================================== warnings summary =======================================================================================
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
  /pagure/tests/__init__.py:13: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp
-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================================================================ 8 passed, 4 warnings in 7.60s ================================================================================
Summary:
  ALL TESTS PASSED
tests/test_pagure_lib_task_services.py
===================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.13, pytest-6.2.2, py-1.11.0, pluggy-0.13.1
rootdir: /pagure
plugins: forked-1.3.0, xdist-2.2.0
gw0 [25] / gw1 [25] / gw2 [25] / gw3 [25]
.........................                                                                                                                                                               [100%]
====================================================================================== warnings summary =======================================================================================
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
tests/__init__.py:13
  /pagure/tests/__init__.py:13: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp
tests/test_pagure_lib_task_services.py: 20 warnings
  /usr/lib/python3.9/site-packages/arrow/arrow.py:703: DeprecationWarning: For compatibility with the datetime.timestamp() method this property will be replaced with a method in the 1.0.0 release, please switch to the .int_timestamp property for identical behaviour as soon as possible.
    warnings.warn(
-- Docs: https://docs.pytest.org/en/stable/warnings.html
============================================================================== 25 passed, 24 warnings in 18.58s ===============================================================================
Summary:
  ALL TESTS PASSED

Can you put more details into the commit message? The detail in the PR message is quite good, and some of it should be in the commit message itself.

rebased onto d08b2b5b9ad67d2fc9b68050b0ef757e1fc17525

Can you put more details into the commit message? The detail in the PR message is quite good, and some of it should be in the commit message itself.

Sure, is the new commit message fine?

Why not just use this in the commit message body?

Config Parameter BROKER_URL (https://docs.pagure.org/pagure/configuration.html#broker-url)
is used by Celery workers and doesn't honor REDIS_PORT and REDIS_DB by default,
it's right now expected that the User overwrite the BROKER_URL parameter if
a non-standard port / db is required.
Both parameters are defined in default_config.py, also based on Celery Docs:
"all fields after the scheme are optional, and will default to localhost on port
6379, using database 0".
Therefore it's safe and more user friendly to change the default of BROKER_URL to:
"redis://%s:%d/%d" % (pagure_config["REDIS_HOST"], pagure_config["REDIS_PORT"], pagure_config["REDIS_DB"])

rebased onto 01a2d6cd4bd6bae67d8ac3580428c14c93e682b8

Why not just use this in the commit message body?

no problem with that, wasn't sure how much information you want in the commit message ;)

rebased onto c88d64dcb5d7d405d63d24f6bd49bd1e8e2f0e84

Two failed tests:

04:39:43  FAILED tests/test_style.py::TestStyle::test_code_with_black - AssertionError:...
04:39:43  FAILED tests/test_style.py::TestStyle::test_code_with_flake8 - AssertionError...

rebased onto 877cf4056c7332100e99123d97e2bdb1d5f50ab3

Two failed tests:

04:39:43 FAILED tests/test_style.py::TestStyle::test_code_with_black - AssertionError:... 04:39:43 FAILED tests/test_style.py::TestStyle::test_code_with_flake8 - AssertionError...

Thanks, makes technically total sense, addressed in the new commit, but weird that it doesn't fail when I run the tests local, have to take a look why that is

Pull-Request has been merged by ngompa
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.