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

feat: use new setting to set ce_source #305

Merged
merged 19 commits into from
Jan 25, 2024
6 changes: 6 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ Change Log
Unreleased
----------

[9.3.0] - 2024-01-24
--------------------
Changed
~~~~~~~
* Allow new EVENTS_SERVICE_NAME setting to override SERVICE_VARIANT for data source.

[9.2.0] - 2023-11-16
--------------------
Added
Expand Down
2 changes: 1 addition & 1 deletion openedx_events/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
more information about the project.
"""

__version__ = "9.2.0"
__version__ = "9.3.0"
16 changes: 15 additions & 1 deletion openedx_events/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ def _ensure_utc_time(_, attribute, value):
raise ValueError(f"'{attribute.name}' must have timezone.utc")


def get_source_name():
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Should be get_service_name.
  2. Can we keep this internal-only and call it _get_service_name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that if this function were private, and just used for this one purpose, it could return SERVICE-NAME-UNSET itself in place of None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to call it from event bus kafka

Copy link
Contributor

Choose a reason for hiding this comment

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

It should still be changed to get_service_name though, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was. I have 2 methods now

"""
Get the value that should be used for the source of an event.
"""
# .. setting_name: EVENTS_SERVICE_NAME
# .. setting_default: None
# .. setting_description: Identifier for the producing/consuming service of an event. Used in setting the source in
# the EventsMetadata. If not set, the EventsMetadata object will look for a SERVICE_VARIANT setting (usually only
# set for lms and cms). The full source will be set to openedx/<EVENTS_SERVICE_NAME or SERVICE_VARIANT>/web.
# If neither variable is set, the source will be "unidentified."
return getattr(settings, "EVENTS_SERVICE_NAME", None) or getattr(settings, "SERVICE_VARIANT", None)


@attr.s(frozen=True)
class EventsMetadata:
"""
Expand Down Expand Up @@ -62,7 +75,8 @@ class EventsMetadata:
source = attr.ib(
type=str, default=None,
converter=attr.converters.default_if_none(
attr.Factory(lambda: "openedx/{service}/web".format(service=getattr(settings, "SERVICE_VARIANT", "")))
attr.Factory(lambda: "openedx/{service}/web".format(service=get_source_name()) if get_source_name()
Copy link
Contributor

Choose a reason for hiding this comment

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

NIt:

  1. I'd find this easier to read if on and earlier line you set:
service_name = "unidentified" or _get_service_name().  # see other PR comment for method name change
  1. Consider something like SERVICE-NAME-UNSET in place of unidentified, to give something easy to search for and easy to resolve.

else "unidentified")
),
validator=attr.validators.instance_of(str),
)
Expand Down
20 changes: 20 additions & 0 deletions openedx_events/tests/test_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@
from datetime import datetime, timezone
from uuid import UUID

import ddt
from django.test import TestCase
from django.test.utils import override_settings

from openedx_events.data import EventsMetadata


@ddt.ddt
class TestEventsMetadata(TestCase):
"""
Tests for the EventsMetadata class.
Expand All @@ -26,3 +29,20 @@ def test_events_metadata_to_and_from_json(self):
as_json = self.metadata.to_json()
from_json = EventsMetadata.from_json(as_json)
self.assertEqual(self.metadata, from_json)

@ddt.data(
('settings_variant', None, 'openedx/settings_variant/web'),
(None, 'my_service', 'openedx/my_service/web'),
(None, None, 'unidentified'),
('settings_variant', 'my_service', 'openedx/my_service/web')
)
@ddt.unpack
def test_events_metadata_source(self, settings_variant, event_bus_service_name, expected_source):
with override_settings(
SERVICE_VARIANT=settings_variant,
EVENTS_SERVICE_NAME=event_bus_service_name,
):
metadata = EventsMetadata(
event_type='test_type'
)
self.assertEqual(metadata.source, expected_source)
Loading