-
Notifications
You must be signed in to change notification settings - Fork 21
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: enable producing to event bus via settings #249
feat: enable producing to event bus via settings #249
Conversation
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
14d23f1
to
10dabba
Compare
10dabba
to
e01833b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera 👍 LGTM
- I tested this: Followed the testing instructions and verified that signals are being sent as per the configuration.
- I read through the code
- I checked for accessibility issues - NA
- Includes documentation
A couple of points I noticed:
fastavro
1.8.0 seems to have some build issues (it failed to build for me on Apple ARM64) and they seems to have fixed it in version 1.8.1. I was able to test it after changingrequirements/dev.txt
to usefastavro==1.8.1
. Maybe it's worth updating it.- I added a suggestion about using Pydantic for config validation. I haven't considered the cost of adding an additional dependency, but, it might be worth considering and it definitely not a blocker here.
c36b2a8
to
b991f04
Compare
@navinkarkera: I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but nothing blocking.
@navinkarkera I think this is ready to merge. I had nits but you can choose to address them or not, they're not blocking. |
Hi @navinkarkera! Just flagging that some branch conflicts have popped up. |
@mphilbrick211: We heard Navin may be on vacation, so we'll hear back when we hear back. |
Yes, thank you, @robrap! I forgot he was away. |
ffc3d29
to
86b74ae
Compare
docs: update adding event to bus document chore: fix lint issues fix: docs and type checking Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com> docs: update events how to fix: Update openedx_events/apps.py Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com> refactor: catch bad event type and log
2c2a3bc
to
e6d8820
Compare
e6d8820
to
4b786f3
Compare
@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description: Implements ADR merged in #224 .
ISSUE: #210
Testing instructions:
make requirements
.python manage.py shell
.EVENT_BUS_PRODUCER_CONFIG
are printed by the stub implementation of event bus producer.Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.