pagure

Clone

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

file modified
+3 -3 
@@ -105,10 +105,10 @@ 
 

  is the broker that is used to communicate between the web application and
 

  its workers.
 

  

- Defaults to: ``'redis://%s' % APP.config['REDIS_HOST']``
 

+ Defaults to: ``"redis://%s:%d/%d" % (pagure_config["REDIS_HOST"], pagure_config["REDIS_PORT"], pagure_config["REDIS_DB"])``
 

  

- .. note:: See the :ref:`redis-section` for the ``REDIS_HOST`` configuration
 

-           key
 

+ .. note:: See the :ref:`redis-section` for the ``REDIS_HOST``, ``REDIS_PORT``
 

+           and ``REDIS_DB``configuration keys
 

  

  

  Repo Directories

file modified
+5 -1 
@@ -49,7 +49,11 @@ 
 

  elif pagure_config.get("BROKER_URL"):
 

      broker_url = pagure_config["BROKER_URL"]
 

  else:
 

-     broker_url = "redis://%s" % pagure_config["REDIS_HOST"]
 

+     broker_url = "redis://%s:%d/%d" % (
 

+         pagure_config["REDIS_HOST"],
 

+         pagure_config["REDIS_PORT"],
 

+         pagure_config["REDIS_DB"],
 

+     )
 

  

  conn = Celery("tasks", broker=broker_url, backend=broker_url)
 

  conn.conf.update(pagure_config["CELERY_CONFIG"])

file modified
+6 -1 
@@ -40,7 +40,12 @@ 
 

  elif pagure_config.get("BROKER_URL"):
 

      broker_url = pagure_config["BROKER_URL"]
 

  else:
 

-     broker_url = "redis://%s" % pagure_config["REDIS_HOST"]
 

+     broker_url = "redis://%s:%d/%d" % (
 

+         pagure_config["REDIS_HOST"],
 

+         pagure_config["REDIS_PORT"],
 

+         pagure_config["REDIS_DB"],
 

+     )
 

+ 

  

  conn = Celery("tasks_mirror", broker=broker_url, backend=broker_url)
 

  conn.conf.update(pagure_config["CELERY_CONFIG"])

file modified
+5 -1 
@@ -45,7 +45,11 @@ 
 

  elif pagure_config.get("BROKER_URL"):
 

      broker_url = pagure_config["BROKER_URL"]
 

  else:
 

-     broker_url = "redis://%s" % pagure_config["REDIS_HOST"]
 

+     broker_url = "redis://%s:%d/%d" % (
 

+         pagure_config["REDIS_HOST"],
 

+         pagure_config["REDIS_PORT"],
 

+         pagure_config["REDIS_DB"],
 

+     )
 

  

  conn = Celery("tasks", broker=broker_url, backend=broker_url)
 

  conn.conf.update(pagure_config["CELERY_CONFIG"])

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
a year ago

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
a year ago

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
a year ago

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 877cf40
a year ago

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
a year ago
Metadata
None
No Tags
Changes Summary 4
+3 -3
file changed
doc/configuration.rst
+5 -1
file changed
pagure/lib/tasks.py
+6 -1
file changed
pagure/lib/tasks_mirror.py
+5 -1
file changed
pagure/lib/tasks_services.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.