From 4b705be996b5a4c9f8ac63867305fd4b5168c81d Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:19:51 +0100 Subject: [PATCH 1/9] Add monstruous function to support old, transitional and new config options --- synapse/config/workers.py | 98 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/synapse/config/workers.py b/synapse/config/workers.py index a5479dfca98b..6d46ec075049 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -14,7 +14,8 @@ # limitations under the License. import argparse -from typing import Any, List, Union +import logging +from typing import Any, Dict, List, Union import attr @@ -42,6 +43,13 @@ Please add ``start_pushers: false`` to the main config """ +_DEPRECATED_WORKER_DUTY_OPTION_USED = """ +The '%s' configuration option is deprecated and will be removed in a future +Synapse version. Please use ``%s: name_of_worker`` instead. +""" + +logger = logging.getLogger(__name__) + def _instance_to_list_converter(obj: Union[str, List[str]]) -> List[str]: """Helper for allowing parsing a string or list of strings to a config @@ -296,6 +304,94 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.worker_name is None and background_tasks_instance == "master" ) or self.worker_name == background_tasks_instance + def _should_this_worker_perform_duty( + self, + config: Dict[str, Any], + legacy_master_option_name: str, + legacy_worker_app_name: str, + new_option_name: str, + ) -> bool: + """ + Figures out whether this worker should perform a certain duty. + + This function is temporary and is only to deal with the complexity + of allowing old, transitional and new configurations all at once. + + Contradictions between the legacy and new part of a transitional configuration + will lead to a ConfigError. + + Parameters: + config: The config dictionary + legacy_master_option_name: The name of a legacy option, whose value is boolean, + specifying whether it's the master that should handle a certain duty. + legacy_worker_app_name: The name of a legacy Synapse worker application name + that would traditionally perform this duty. + new_option_name: The name of the new option, whose value is the name of a + designated worker to perform the duty. + """ + + # None means 'unspecified'; True means 'run here' and False means + # 'don't run here'. + new_option_should_run_here = None + if new_option_name in config: + designated_worker = config[new_option_name] or "master" + new_option_should_run_here = ( + designated_worker == "master" and self.worker_name is None + ) or designated_worker == self.worker_name + + legacy_option_should_run_here = None + if legacy_master_option_name in config: + run_on_master = bool(config[legacy_master_option_name]) + + legacy_option_should_run_here = ( + self.worker_name is None and run_on_master + ) or (self.worker_app == legacy_worker_app_name and not run_on_master) + + # Suggest using the new option instead. + logger.warning( + _DEPRECATED_WORKER_DUTY_OPTION_USED, + legacy_master_option_name, + new_option_name, + ) + + if self.worker_app == legacy_worker_app_name and config.get( + legacy_master_option_name, True + ): + # As an extra bit of complication, we need to check that the + # specialised worker is only used if the legacy config says the + # master isn't performing the duties. + raise ConfigError( + f"Cannot use deprecated worker app type '{legacy_worker_app_name}' whilst deprecated option '{legacy_master_option_name}' is not set to false.\n" + f"Consider setting `worker_app: synapse.app.generic_worker` and using the '{new_option_name}' option instead.\n" + f"The '{new_option_name}' option replaces '{legacy_master_option_name}'." + ) + + if new_option_should_run_here is None and legacy_option_should_run_here is None: + # Neither option specified; the fallback behaviour is to run on the main process + return self.worker_name is None + + if ( + new_option_should_run_here is not None + and legacy_option_should_run_here is not None + ): + # Both options specified; ensure they match! + if new_option_should_run_here != legacy_option_should_run_here: + update_worker_type = ( + " and set worker_app: synapse.app.generic_worker" + if self.worker_app == legacy_worker_app_name + else "" + ) + # If the values conflict, we suggest the admin removes the legacy option + # for simplicity. + raise ConfigError( + f"Conflicting configuration options: {legacy_master_option_name} (legacy), {new_option_name} (new).\n" + f"Suggestion: remove {legacy_master_option_name}{update_worker_type}.\n" + ) + + # We've already validated that these aren't conflicting; now just see if + # either is True. + return bool(new_option_should_run_here or legacy_option_should_run_here) + def generate_config_section(self, **kwargs: Any) -> str: return """\ ## Workers ## From 3b95f546cdd596609489ce91c90f3014dc2f9a27 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:19:59 +0100 Subject: [PATCH 2/9] Add tests for the monstruous function --- tests/config/test_workers.py | 264 +++++++++++++++++++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 tests/config/test_workers.py diff --git a/tests/config/test_workers.py b/tests/config/test_workers.py new file mode 100644 index 000000000000..6a0f2cc62688 --- /dev/null +++ b/tests/config/test_workers.py @@ -0,0 +1,264 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from typing import Optional +from unittest.mock import Mock + +from synapse.config import ConfigError +from synapse.config.workers import WorkerConfig + +from tests.unittest import TestCase + + +class WorkerDutyConfigTestCase(TestCase): + def _make_worker_config( + self, worker_app: str, worker_name: Optional[str] + ) -> WorkerConfig: + root_config = Mock() + root_config.worker_app = worker_app + root_config.worker_name = worker_name + worker_config = WorkerConfig(root_config) + worker_config.read_config( + {"worker_name": worker_name, "worker_app": worker_app} + ) + return worker_config + + def test_old_configs_master(self) -> None: + """ + Tests old (legacy) config options. This is for the master's config. + """ + main_process_config = self._make_worker_config( + worker_app="synapse.app.homeserver", worker_name=None + ) + + self.assertTrue( + main_process_config._should_this_worker_perform_duty( + {}, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + self.assertTrue( + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": True, + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + self.assertFalse( + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + def test_old_configs_appservice_worker(self) -> None: + """ + Tests old (legacy) config options. This is for the worker's config. + """ + appservice_worker_config = self._make_worker_config( + worker_app="synapse.app.appservice", worker_name="worker1" + ) + + with self.assertRaises(ConfigError): + # This raises because you need to set notify_appservices: False + # before using the synapse.app.appservice worker type + self.assertFalse( + appservice_worker_config._should_this_worker_perform_duty( + {}, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + with self.assertRaises(ConfigError): + # This also raises because you need to set notify_appservices: False + # before using the synapse.app.appservice worker type + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices": True, + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + + self.assertTrue( + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + def test_transitional_configs_master(self) -> None: + """ + Tests transitional (legacy + new) config options. This is for the master's config. + """ + + main_process_config = self._make_worker_config( + worker_app="synapse.app.homeserver", worker_name=None + ) + + self.assertTrue( + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": True, + "notify_appservices_from_worker": "master", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + self.assertFalse( + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + "notify_appservices_from_worker": "worker1", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + with self.assertRaises(ConfigError): + # Contradictory because we say the master should notify appservices, + # then we say worker1 is the designated worker to do that! + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": True, + "notify_appservices_from_worker": "worker1", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + + with self.assertRaises(ConfigError): + # Contradictory because we say the master shouldn't notify appservices, + # then we say master is the designated worker to do that! + main_process_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + "notify_appservices_from_worker": "master", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + + def test_transitional_configs_appservice_worker(self) -> None: + """ + Tests transitional (legacy + new) config options. This is for the worker's config. + """ + appservice_worker_config = self._make_worker_config( + worker_app="synapse.app.appservice", worker_name="worker1" + ) + + self.assertTrue( + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + "notify_appservices_from_worker": "worker1", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + with self.assertRaises(ConfigError): + # This raises because this worker is the appservice app type, yet + # another worker is the designated worker! + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices": False, + "notify_appservices_from_worker": "worker2", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + + def test_new_configs_master(self) -> None: + """ + Tests new config options. This is for the master's config. + """ + main_process_config = self._make_worker_config( + worker_app="synapse.app.homeserver", worker_name=None + ) + + self.assertTrue( + main_process_config._should_this_worker_perform_duty( + {"notify_appservices_from_worker": None}, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + self.assertFalse( + main_process_config._should_this_worker_perform_duty( + {"notify_appservices_from_worker": "worker1"}, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + def test_new_configs_appservice_worker(self) -> None: + """ + Tests new config options. This is for the worker's config. + """ + appservice_worker_config = self._make_worker_config( + worker_app="synapse.app.generic_worker", worker_name="worker1" + ) + + self.assertTrue( + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices_from_worker": "worker1", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) + + self.assertFalse( + appservice_worker_config._should_this_worker_perform_duty( + { + "notify_appservices_from_worker": "worker2", + }, + "notify_appservices", + "synapse.app.appservice", + "notify_appservices_from_worker", + ) + ) From 8be9be8a5d5b292bc5ec1883c5bd30162c1a6432 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:25:20 +0100 Subject: [PATCH 3/9] Convert notify_appservices to notify_appservices_from_worker --- synapse/app/generic_worker.py | 16 ---------------- synapse/config/appservice.py | 1 - synapse/config/workers.py | 7 +++++++ synapse/handlers/appservice.py | 2 +- 4 files changed, 8 insertions(+), 18 deletions(-) diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index 1865c671f41c..07dddc0b1326 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -441,22 +441,6 @@ def start(config_options: List[str]) -> None: "synapse.app.user_dir", ) - if config.worker.worker_app == "synapse.app.appservice": - if config.appservice.notify_appservices: - sys.stderr.write( - "\nThe appservices must be disabled in the main synapse process" - "\nbefore they can be run in a separate worker." - "\nPlease add ``notify_appservices: false`` to the main config" - "\n" - ) - sys.exit(1) - - # Force the appservice to start since they will be disabled in the main config - config.appservice.notify_appservices = True - else: - # For other worker types we force this to off. - config.appservice.notify_appservices = False - if config.worker.worker_app == "synapse.app.user_dir": if config.server.update_user_directory: sys.stderr.write( diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 720b90a28386..b13cc6bb6e4f 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -33,7 +33,6 @@ class AppServiceConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.app_service_config_files = config.get("app_service_config_files", []) - self.notify_appservices = config.get("notify_appservices", True) self.track_appservice_user_ips = config.get("track_appservice_user_ips", False) def generate_config_section(cls, **kwargs: Any) -> str: diff --git a/synapse/config/workers.py b/synapse/config/workers.py index 6d46ec075049..b5d1da400b07 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -304,6 +304,13 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.worker_name is None and background_tasks_instance == "master" ) or self.worker_name == background_tasks_instance + self.should_notify_appservices = self._should_this_worker_perform_duty( + config, + legacy_master_option_name="notify_appservices", + legacy_worker_app_name="synapse.app.appservice", + new_option_name="notify_appservices_from_worker", + ) + def _should_this_worker_perform_duty( self, config: Dict[str, Any], diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index b3894666ccf5..85bd5e47682b 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -59,7 +59,7 @@ def __init__(self, hs: "HomeServer"): self.scheduler = hs.get_application_service_scheduler() self.started_scheduler = False self.clock = hs.get_clock() - self.notify_appservices = hs.config.appservice.notify_appservices + self.notify_appservices = hs.config.worker.should_notify_appservices self.event_sources = hs.get_event_sources() self._msc2409_to_device_messages_enabled = ( hs.config.experimental.msc2409_to_device_messages_enabled From 6553c3502f765b38919ae056f555a02efcf14801 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:40:06 +0100 Subject: [PATCH 4/9] Fix up Docker worker script --- docker/configure_workers_and_start.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/configure_workers_and_start.py b/docker/configure_workers_and_start.py index 3bda6c300ba8..6ffa82ae0e7d 100755 --- a/docker/configure_workers_and_start.py +++ b/docker/configure_workers_and_start.py @@ -69,10 +69,10 @@ "worker_extra_conf": "enable_media_repo: true", }, "appservice": { - "app": "synapse.app.appservice", + "app": "synapse.app.generic_worker", "listener_resources": [], "endpoint_patterns": [], - "shared_extra_conf": {"notify_appservices": False}, + "shared_extra_conf": {"notify_appservices_from_worker": "appservice"}, "worker_extra_conf": "", }, "federation_sender": { From 22ec2ed6352c4541d3db5537dfc4c98dc1af75a8 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:40:20 +0100 Subject: [PATCH 5/9] Fix up documentation with new style --- docs/workers.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/workers.md b/docs/workers.md index afdcd785e408..1d049b6c4f28 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -435,6 +435,23 @@ An example for a dedicated background worker instance: {{#include systemd-with-workers/workers/background_worker.yaml}} ``` +#### Notifying Application Services + +You can designate one worker to send output traffic to Application Services. + +Specify its name in the shared configuration as follows: + +```yaml +notify_appservices_from_worker: worker_name +``` + +This work cannot be load-balanced; please ensure the main process is restarted +after setting this option in the shared configuration! + +This style of configuration supersedes the legacy `synapse.app.appservice` +worker application type. + + ### `synapse.app.pusher` Handles sending push notifications to sygnal and email. Doesn't handle any @@ -453,6 +470,9 @@ pusher_instances: ### `synapse.app.appservice` +**Deprecated as of Synapse v1.58.** [Use `synapse.app.generic_worker` with the +`notify_appservices_from_worker` option instead.](#notifying-application-services) + Handles sending output traffic to Application Services. Doesn't handle any REST endpoints itself, but you should set `notify_appservices: False` in the shared configuration file to stop the main synapse sending appservice notifications. From cc0242472f0fa70b22385c64e5404087e773285a Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 16:55:02 +0100 Subject: [PATCH 6/9] Add some upgrade notes --- docs/upgrade.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/upgrade.md b/docs/upgrade.md index 3a8aeb039533..f44dd0ddd72a 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -89,6 +89,33 @@ process, for example: dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb ``` +# Upgrading to v1.59.0 + +## Deprecation of the `synapse.app.appservice` worker application type + +The `synapse.app.appservice` worker application type allowed you to configure a +single worker to use to notify application services of new events, as long +as this functionality was disabled on the main process with `notify_appservices: False`. + +To unify Synapse's worker types, the `synapse.app.appservice` worker application +type and the `notify_appservices` configuration option have been deprecated. + +To get the same functionality, it's now recommended that the `synapse.app.generic_worker` +worker application type is used and that the `notify_appservices_from_worker` option +is set to the name of a worker. + +For the time being, `notify_appservices_from_worker` can be used alongside +`synapse.app.appservice` and `notify_appservices` to make it easier to transition +between the two configurations, however please note that: + +- the options must not contradict each other (otherwise Synapse won't start); and +- the `notify_appservices` option will be removed in a future release of Synapse. + +Please see [the relevant section of the worker documentation][v1_59_notify_ases_from] for more information. + +[v1_59_notify_ases_from]: workers.md#notifying-application-services + + # Upgrading to v1.58.0 ## Groups/communities feature has been disabled by default @@ -96,6 +123,7 @@ process, for example: The non-standard groups/communities feature in Synapse has been disabled by default and will be removed in Synapse v1.61.0. + # Upgrading to v1.57.0 ## Changes to database schema for application services From 2fddb21816f49bf5b52351159c416dd8b2e6d538 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 12 Apr 2022 17:00:14 +0100 Subject: [PATCH 7/9] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/12452.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12452.feature diff --git a/changelog.d/12452.feature b/changelog.d/12452.feature new file mode 100644 index 000000000000..22f054d34493 --- /dev/null +++ b/changelog.d/12452.feature @@ -0,0 +1 @@ +Add the `notify_appservices_from_worker` configuration option (superseding `notify_appservices`) to allow a generic worker to be designated as the worker to send traffic to Application Services. \ No newline at end of file From 2dd44f66f5ae1ce570dabf7c1fb56def6b046aeb Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 27 Apr 2022 10:38:34 +0100 Subject: [PATCH 8/9] Prevent tests from raising exception whilst setting up the config --- tests/config/test_workers.py | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/tests/config/test_workers.py b/tests/config/test_workers.py index 6a0f2cc62688..33a64a888519 100644 --- a/tests/config/test_workers.py +++ b/tests/config/test_workers.py @@ -11,26 +11,36 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Optional +from typing import Any, Mapping, Optional from unittest.mock import Mock +from frozendict import frozendict + from synapse.config import ConfigError from synapse.config.workers import WorkerConfig from tests.unittest import TestCase +_EMPTY_FROZENDICT: Mapping[str, Any] = frozendict() + class WorkerDutyConfigTestCase(TestCase): def _make_worker_config( - self, worker_app: str, worker_name: Optional[str] + self, + worker_app: str, + worker_name: Optional[str], + extras: Mapping[str, Any] = _EMPTY_FROZENDICT, ) -> WorkerConfig: root_config = Mock() root_config.worker_app = worker_app root_config.worker_name = worker_name worker_config = WorkerConfig(root_config) - worker_config.read_config( - {"worker_name": worker_name, "worker_app": worker_app} - ) + worker_config_dict = { + "worker_name": worker_name, + "worker_app": worker_app, + **extras, + } + worker_config.read_config(worker_config_dict) return worker_config def test_old_configs_master(self) -> None: @@ -77,7 +87,13 @@ def test_old_configs_appservice_worker(self) -> None: Tests old (legacy) config options. This is for the worker's config. """ appservice_worker_config = self._make_worker_config( - worker_app="synapse.app.appservice", worker_name="worker1" + worker_app="synapse.app.appservice", + worker_name="worker1", + extras={ + # Set notify_appservices to false for the initialiser's config, + # so that it doesn't raise an exception here. + "notify_appservices": False, + }, ) with self.assertRaises(ConfigError): @@ -179,7 +195,13 @@ def test_transitional_configs_appservice_worker(self) -> None: Tests transitional (legacy + new) config options. This is for the worker's config. """ appservice_worker_config = self._make_worker_config( - worker_app="synapse.app.appservice", worker_name="worker1" + worker_app="synapse.app.appservice", + worker_name="worker1", + extras={ + # Set notify_appservices to false for the initialiser's config, + # so that it doesn't raise an exception here. + "notify_appservices": False, + }, ) self.assertTrue( From ec0c07b92539c3f6cd094c9b6d792b4099bb33c9 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 5 May 2022 14:12:13 +0100 Subject: [PATCH 9/9] Address feedback from code review --- synapse/config/workers.py | 6 +++++- tests/config/test_workers.py | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/config/workers.py b/synapse/config/workers.py index b5d1da400b07..a9dbcc6d3dc7 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -331,10 +331,13 @@ def _should_this_worker_perform_duty( config: The config dictionary legacy_master_option_name: The name of a legacy option, whose value is boolean, specifying whether it's the master that should handle a certain duty. - legacy_worker_app_name: The name of a legacy Synapse worker application name + e.g. "notify_appservices" + legacy_worker_app_name: The name of a legacy Synapse worker application that would traditionally perform this duty. + e.g. "synapse.app.appservice" new_option_name: The name of the new option, whose value is the name of a designated worker to perform the duty. + e.g. "notify_appservices_from_worker" """ # None means 'unspecified'; True means 'run here' and False means @@ -397,6 +400,7 @@ def _should_this_worker_perform_duty( # We've already validated that these aren't conflicting; now just see if # either is True. + # (By this point, these are either the same value or only one is not None.) return bool(new_option_should_run_here or legacy_option_should_run_here) def generate_config_section(self, **kwargs: Any) -> str: diff --git a/tests/config/test_workers.py b/tests/config/test_workers.py index 33a64a888519..da81bb9655fc 100644 --- a/tests/config/test_workers.py +++ b/tests/config/test_workers.py @@ -92,6 +92,7 @@ def test_old_configs_appservice_worker(self) -> None: extras={ # Set notify_appservices to false for the initialiser's config, # so that it doesn't raise an exception here. + # (This is not read by `_should_this_worker_perform_duty`.) "notify_appservices": False, }, ) @@ -200,6 +201,7 @@ def test_transitional_configs_appservice_worker(self) -> None: extras={ # Set notify_appservices to false for the initialiser's config, # so that it doesn't raise an exception here. + # (This is not read by `_should_this_worker_perform_duty`.) "notify_appservices": False, }, )