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!: Only accept metadata object in send_event_with_custom_metadata #179

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

timmc-edx
Copy link
Contributor

This changes send_event_with_custom_metadata to accept a single metadata object rather than all the fields that go into one.

It turns out that in practice, the event-bus-kafka consumer already needs to build an EventsMetadata object or something equivalent to it as it reconstructs the metadata from Kafka message headers, since that's where the parsing and validation logic already is. It then has to splat the object back out when calling this method. This leads to a possible collision risk in the method's kwargs, but it's also just awkward. It is likely that the same issue would arise in any other consumer.

Merge checklist:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

This changes `send_event_with_custom_metadata` to accept a single
`metadata` object rather than all the fields that go into one.

It turns out that in practice, the event-bus-kafka consumer already needs
to build an EventsMetadata object or something equivalent to it as it
reconstructs the metadata from Kafka message headers, since that's where
the parsing and validation logic already is. It then has to splat the
object back out when calling this method. This leads to a possible
collision risk in the method's kwargs, but it's also just awkward. It is
likely that the same issue would arise in any other consumer.
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM. Good catch.

openedx_events/tooling.py Outdated Show resolved Hide resolved
@timmc-edx timmc-edx merged commit 2a9eebd into main Feb 3, 2023
@timmc-edx timmc-edx deleted the timmc/custom2 branch February 3, 2023 18:06
mariajgrimaldi pushed a commit that referenced this pull request Feb 15, 2023
…a` (#179)

This changes `send_event_with_custom_metadata` to accept a single
`metadata` object rather than all the fields that go into one.

It turns out that in practice, the event-bus-kafka consumer already needs
to build an EventsMetadata object or something equivalent to it as it
reconstructs the metadata from Kafka message headers, since that's where
the parsing and validation logic already is. It then has to splat the
object back out when calling this method. This leads to a possible
collision risk in the method's kwargs, but it's also just awkward. It is
likely that the same issue would arise in any other consumer.

Addresses #178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants