refactor!: Move to new package structure to highlight public/internal…

… distinction (#37) Document the public API in the README; add ADR to explain refactor. Also: - Fix some rST syntax in the changelog; fix a release heading - Add another missing `__init__.py` file (and fix lint exposed by that) - Small test improvement while I'm in there - Remove redundant consumer README (duplicated by main README)
openedx · Sep 1, 2022 · 4444d3f · 4444d3f
1 parent 94d7c9f
commit 4444d3f
Show file tree

Hide file tree

Showing 18 changed files with 99 additions and 28 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -16,7 +16,15 @@ Unreleased
 
 *
 
-[0.5.0] - 2022-08-31
+[0.6.0] - 2022-09-01
+********************
+
+Changed
+=======
+
+* **Breaking change**: Public API is now defined in ``edx_event_bus_kafka`` package and ``edx_event_bus_kafka.management.commands`` package; all other modules should be considered unstable and not for external use.
+
+[0.5.1] - 2022-08-31
 ********************
 
 Fixed
@@ -32,8 +40,8 @@ Changed
 
 * **Breaking changes** in the producer module, refactored to expose a better API:
 
-  * Rather than `send_to_event_bus(...)`, relying code should now call `get_producer().send(...)`.
-  * The `sync` kwarg is gone; to flush and sync messages before shutdown, call `get_producer().prepare_for_shutdown()` instead.
+  * Rather than ``send_to_event_bus(...)``, relying code should now call ``get_producer().send(...)``.
+  * The ``sync`` kwarg is gone; to flush and sync messages before shutdown, call ``get_producer().prepare_for_shutdown()`` instead.
 
 * Clarify that config module is for internal use only.
 * Implementation changes: Only a single Producer is created, and is used for all signals.

diff --git a/README.rst b/README.rst
@@ -33,6 +33,9 @@ For manual testing, see `<docs/how_tos/manual_testing.rst>`__.
 Documentation
 *************
 
+- Main API: ``edx_event_bus_kafka`` exposes ``get_producer`` and a Producer API class. See `<https://github.com/openedx/openedx-events/issues/87>`_ for how these will be documented and used in the future.
+- Django management commands: ``edx_event_bus_kafka.management.commands.*`` expose ``Command`` classes
+
 OEP-52 documentation: https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0052-arch-event-bus-architecture.html
 (TODO: `Set up documentation <https://openedx.atlassian.net/wiki/spaces/DOC/pages/21627535/Publish+Documentation+on+Read+the+Docs>`_)
 

diff --git a/docs/decisions/0006-public-api-and-app-organization.rst b/docs/decisions/0006-public-api-and-app-organization.rst
@@ -0,0 +1,47 @@
+6. Public API and App Organization
+##################################
+
+
+Status
+******
+
+Accepted
+
+Context
+*******
+
+There has been no clear distinction between the public API in this package and the various internal, subject-to-change module attributes. This means we are more likely to end up with breaking changes, possibly without knowing it. Callers may also be unclear about which functions and classes they should be using.
+
+Decision
+********
+
+Put most of the code into packages with ``internal`` in their name, and import the public bits into modules that are not marked that way.
+
+- ``edx_event_bus_kafka/__init__.py`` imports various producer and consumer module attributes
+- ``edx_event_bus_kafka/management/commands/*.py`` expose ``Command`` classes
+
+These will also be documented in the main README.
+
+The benefits of this setup include:
+
+* A clear designation of what is part of the public API.
+* The ability to refactor the implementation without changing the API.
+* A clear reminder to developers adding new code that it needs to be exposed if it is public.
+* A clear reminder to developers using the library not to use code from the internal implementation.
+
+Consequences
+************
+
+Whenever a new class or function is added to the edx_django_utils public API, it should be implemented in the Django app's ``internal`` module and explicitly imported in its ``__init__.py`` module.
+
+There will be some additional friction when a developer looks up the docstrings of a public API element, since they'll first have to open the public module and then find the internal module. Having the public docstrings in a module marked "internal" also creates some dissonance. Our prediction is that this dissonance will be outweighed by the benefits of API stability.
+
+Rejected Alternatives
+*********************
+
+None.
+
+References
+**********
+
+This draws heavily on an ADR in ``edx-django-utils``: `<https://github.com/openedx/edx-django-utils/blob/master/docs/decisions/0004-public-api-and-app-organization.rst>`_
diff --git a/edx_event_bus_kafka/__init__.py b/edx_event_bus_kafka/__init__.py
@@ -1,5 +1,11 @@
 """
 Kafka implementation for Open edX event bus.
+
+Public API will be in this module for the most part.
+
+See ADR ``docs/decisions/0006-public-api-and-app-organization.rst`` for the reasoning.
 """
 
-__version__ = '0.5.1'
+from edx_event_bus_kafka.internal.producer import EventProducerKafka, get_producer
+
+__version__ = '0.6.0'
diff --git a/edx_event_bus_kafka/consumer/README.rst b/edx_event_bus_kafka/consumer/README.rst
diff --git a/edx_event_bus_kafka/consumer/__init__.py b/edx_event_bus_kafka/consumer/__init__.py
diff --git a/edx_event_bus_kafka/internal/__init__.py b/edx_event_bus_kafka/internal/__init__.py
@@ -0,0 +1,7 @@
+"""
+Most of the implementation of the package is here and is internal-only.
+
+Public API will be in the ``edx_event_bus_kafka`` module for the most part.
+
+See ADR ``docs/decisions/0006-public-api-and-app-organization.rst`` for the reasoning.
+"""
diff --git a/edx_event_bus_kafka/config.py → edx_event_bus_kafka/internal/config.py b/edx_event_bus_kafka/config.py → edx_event_bus_kafka/internal/config.py
diff --git a/...vent_bus_kafka/consumer/event_consumer.py → edx_event_bus_kafka/internal/consumer.py b/...vent_bus_kafka/consumer/event_consumer.py → edx_event_bus_kafka/internal/consumer.py
@@ -13,7 +13,7 @@
 from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED
 from openedx_events.tooling import OpenEdxPublicSignal
 
-from edx_event_bus_kafka.config import get_schema_registry_client, load_common_settings
+from .config import get_schema_registry_client, load_common_settings
 
 logger = logging.getLogger(__name__)
 

diff --git a/...nt_bus_kafka/publishing/event_producer.py → edx_event_bus_kafka/internal/producer.py b/...nt_bus_kafka/publishing/event_producer.py → edx_event_bus_kafka/internal/producer.py
@@ -14,7 +14,7 @@
 from openedx_events.event_bus.avro.serializer import AvroSignalSerializer
 from openedx_events.tooling import OpenEdxPublicSignal
 
-from edx_event_bus_kafka.config import get_schema_registry_client, load_common_settings
+from .config import get_schema_registry_client, load_common_settings
 
 logger = logging.getLogger(__name__)
 

diff --git a/...vent_bus_kafka/consumer/tests/__init__.py → ...vent_bus_kafka/internal/tests/__init__.py b/...vent_bus_kafka/consumer/tests/__init__.py → ...vent_bus_kafka/internal/tests/__init__.py
diff --git a/edx_event_bus_kafka/tests/test_config.py → ...t_bus_kafka/internal/tests/test_config.py b/edx_event_bus_kafka/tests/test_config.py → ...t_bus_kafka/internal/tests/test_config.py
@@ -6,7 +6,7 @@
 
 from django.test.utils import override_settings
 
-from edx_event_bus_kafka import config
+from edx_event_bus_kafka.internal import config
 
 # See https://github.com/openedx/event-bus-kafka/blob/main/docs/decisions/0005-optional-import-of-confluent-kafka.rst
 try:
@@ -16,15 +16,24 @@
 
 
 class TestSchemaRegistryClient(TestCase):
+    """
+    Test client creation.
+    """
     def test_unconfigured(self):
         assert config.get_schema_registry_client() is None
 
     def test_configured(self):
         with override_settings(EVENT_BUS_KAFKA_SCHEMA_REGISTRY_URL='http://localhost:12345'):
             assert isinstance(config.get_schema_registry_client(), SchemaRegistryClient)
 
+        # Check if it's cached, too
+        assert config.get_schema_registry_client() is config.get_schema_registry_client()
+
 
 class TestCommonSettings(TestCase):
+    """
+    Test loading of settings common to producer and consumer.
+    """
     def test_unconfigured(self):
         assert config.load_common_settings() is None
 

diff --git a/...fka/consumer/tests/test_event_consumer.py → ...bus_kafka/internal/tests/test_consumer.py b/...fka/consumer/tests/test_event_consumer.py → ...bus_kafka/internal/tests/test_consumer.py
@@ -11,7 +11,7 @@
 from openedx_events.learning.data import UserData, UserPersonalData
 from openedx_events.tooling import OpenEdxPublicSignal
 
-from edx_event_bus_kafka.consumer.event_consumer import KafkaEventConsumer
+from edx_event_bus_kafka.internal.consumer import KafkaEventConsumer
 from edx_event_bus_kafka.management.commands.consume_events import Command
 
 
@@ -126,7 +126,7 @@ class TestCommand(TestCase):
     """
 
     @override_settings(EVENT_BUS_KAFKA_CONSUMERS_ENABLED=False)
-    @patch('edx_event_bus_kafka.consumer.event_consumer.KafkaEventConsumer._create_consumer')
+    @patch('edx_event_bus_kafka.internal.consumer.KafkaEventConsumer._create_consumer')
     def test_kafka_consumers_disabled(self, mock_create_consumer):
         call_command(Command(), topic='test', group_id='test', signal='')
         assert not mock_create_consumer.called
diff --git a/...s_kafka/publishing/test_event_producer.py → ...bus_kafka/internal/tests/test_producer.py b/...s_kafka/publishing/test_event_producer.py → ...bus_kafka/internal/tests/test_producer.py
@@ -12,7 +12,7 @@
 from openedx_events.event_bus.avro.serializer import AvroSignalSerializer
 from openedx_events.learning.data import UserData, UserPersonalData
 
-import edx_event_bus_kafka.publishing.event_producer as ep
+import edx_event_bus_kafka.internal.producer as ep
 
 # See https://github.com/openedx/event-bus-kafka/blob/main/docs/decisions/0005-optional-import-of-confluent-kafka.rst
 try:
@@ -94,7 +94,7 @@ def test_get_producer_configured(self):
         ):
             assert isinstance(ep.get_producer(), ep.EventProducerKafka)
 
-    @patch('edx_event_bus_kafka.publishing.event_producer.logger')
+    @patch('edx_event_bus_kafka.internal.producer.logger')
     def test_on_event_deliver(self, mock_logger):
         fake_event = MagicMock()
         fake_event.topic.return_value = 'some_topic'
@@ -112,7 +112,7 @@ def test_on_event_deliver(self, mock_logger):
     # Mock out the serializers for this one so we don't have to deal
     # with expected Avro bytes -- and they can't call their schema server.
     @patch(
-        'edx_event_bus_kafka.publishing.event_producer.get_serializers', autospec=True,
+        'edx_event_bus_kafka.internal.producer.get_serializers', autospec=True,
         return_value=(
             lambda _key, _ctx: b'key-bytes-here',
             lambda _value, _ctx: b'value-bytes-here',

diff --git a/edx_event_bus_kafka/management/commands/__init__.py b/edx_event_bus_kafka/management/commands/__init__.py
@@ -0,0 +1,5 @@
+"""
+Management commands for the Kafka implementation.
+
+(Django requires this package structure.)
+"""
diff --git a/edx_event_bus_kafka/management/commands/consume_events.py b/edx_event_bus_kafka/management/commands/consume_events.py
@@ -1,7 +1,5 @@
 """
 Makes ``consume_events`` management command available.
-
-Implements required ``APP.management.commands.*.Command`` structure.
 """
 
-from edx_event_bus_kafka.consumer.event_consumer import ConsumeEventsCommand as Command  # pylint: disable=unused-import
+from edx_event_bus_kafka.internal.consumer import ConsumeEventsCommand as Command  # pylint: disable=unused-import
diff --git a/edx_event_bus_kafka/management/commands/produce_event.py b/edx_event_bus_kafka/management/commands/produce_event.py
@@ -10,7 +10,7 @@
 from django.core.management.base import BaseCommand
 from django.utils.module_loading import import_string
 
-from edx_event_bus_kafka.publishing.event_producer import get_producer
+from edx_event_bus_kafka.internal.producer import get_producer
 
 logger = logging.getLogger(__name__)
 

diff --git a/edx_event_bus_kafka/publishing/__init__.py b/edx_event_bus_kafka/publishing/__init__.py