Skip to content

feat(ISV-6803): mercury filters github events#18

Merged
tomasbakk merged 2 commits into
mainfrom
ISV-6803
Mar 31, 2026
Merged

feat(ISV-6803): mercury filters github events#18
tomasbakk merged 2 commits into
mainfrom
ISV-6803

Conversation

@tomasbakk
Copy link
Copy Markdown
Contributor

Closes ISV-6803


This is an extension of a BaseConsumer of an additional filter, that checks for the messages and returns True/False based on whether the user-provided function was used or no.

This is the general mechanism, checking for the headers themselves was not implemented/hardcoded. As I understood it, the user-provided function will be provided. But if needed, I can extend it to actually check for headers and the message (and format) inside.

@tomasbakk tomasbakk changed the title [ISV-6803] mercury filters github events feat(ISV-6803): mercury filters github events Mar 30, 2026
Comment thread tests/integration/test_user_function_filter.py Outdated
Copy link
Copy Markdown
Contributor

@BorekZnovustvoritel BorekZnovustvoritel left a comment

Choose a reason for hiding this comment

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

LGTM, I just noticed we should address some tech debt (not stemming from your code, but you do use the same pattern as the one that could cause problems) and I have also 2 other suggestions for improvement. Let me know if you want to address the tech debt (it's literally just deleting 3 lines) or if I should do it.

Comment thread src/retriable_kafka_client/consumer.py Outdated
Comment thread src/retriable_kafka_client/consumer.py Outdated
Comment thread tests/integration/test_user_function_filter.py Outdated
Copy link
Copy Markdown
Contributor

@bclindner bclindner left a comment

Choose a reason for hiding this comment

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

i don't see anything out of place besides what @BorekZnovustvoritel mentioned, this looks good to me 👍🏻

@tomasbakk
Copy link
Copy Markdown
Contributor Author

tomasbakk commented Mar 30, 2026

LGTM, I just noticed we should address some tech debt (not stemming from your code, but you do use the same pattern as the one that could cause problems) and I have also 2 other suggestions for improvement. Let me know if you want to address the tech debt (it's literally just deleting 3 lines) or if I should do it.

@BorekZnovustvoritel Sure, I can go through the code and adjust it. Is there anything else on top of what you've talked about in this paragraph or is it just this? Tech debt This is not a problem with your code, I just realized that direct calling of _self.consumer.commit() may create inconsistencies...

@BorekZnovustvoritel
Copy link
Copy Markdown
Contributor

Just removing the other direct commits should be enough. We only want the commit schedule to take care of commits and the schedule doesn't mind gaps (skipped messages).

@tomasbakk
Copy link
Copy Markdown
Contributor Author

@BorekZnovustvoritel I applied the suggestions. Thanks for the review!

Copy link
Copy Markdown
Contributor

@BorekZnovustvoritel BorekZnovustvoritel left a comment

Choose a reason for hiding this comment

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

No objections, thank you!

@tomasbakk tomasbakk merged commit cf4a4d1 into main Mar 31, 2026
5 checks passed
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.

3 participants