8000 Added EVENT_RULE_ENGINE flag based opt-in by zaingz · Pull Request #11816 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Added EVENT_RULE_ENGINE flag based opt-in #11816

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 19, 2024
Merged

Added EVENT_RULE_ENGINE flag based opt-in #11816

merged 5 commits into from
Nov 19, 2024

Conversation

zaingz
Copy link
Contributor
@zaingz zaingz commented Nov 8, 2024 •
edited
Loading

Motivation

This PR adds support for a Python-based event rule engine as an alternative to the Java implementation.
This addresses installation issues some users face with the Java event ruler (GH-11299), particularly
in environments with proxy setups or SSL certificate issues.

Changes

  • Created new utility event_matcher.py to centralize event matching logic
  • Supports both string (EventBridge) and dict (ESM/Pipes) inputs
  • Maintains Java engine as default but allows configuration via EVENT_RULE_ENGINE
  • Updated references in:
    • events/provider.py
    • events/v1/provider.py
    • lambda_/event_source_mapping/pollers/poller.py
  • Added unit test

Testing

  1. Run the new test suite:
pytest tests/unit/utils/test_event_matcher.py

@github-actions GitHub Actions
Copy link
github-actions bot commented Nov 8, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 43m 50s ⏱️ -11s
3 526 tests ±0  3 133 ✅ ±0  393 💤 ±0  0 ❌ ±0 
3 528 runs  ±0  3 133 ✅ ±0  395 💤 ±0  0 ❌ ±0 

Results for commit fd34e22. ± Comparison against base commit 5b44747.

♻️ This comment has been updated with latest results.

@zaingz zaingz force-pushed the events-rule-config-optin branch from f88fe69 to 3bd6b1b Compare November 8, 2024 20:46
@zaingz zaingz changed the title [WIP] Added EVENT_RULE_ENGINE flag based opt-in Added EVENT_RULE_ENGINE flag based opt-in Nov 11, 2024
joe4dev
joe4dev reviewed Nov 11, 2024
View reviewed changes
Copy link
Member
@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Nice centralization and addition of unit tests (which we probably under-utilize) 👍

Feel free to add the change type label and self-assign the PR.

Can we trigger the CI run here by authenticating with CircleCI?

@zaingz zaingz added this to the 4.0 milestone Nov 13, 2024
@zaingz zaingz added aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 13, 2024
@zaingz zaingz self-assigned this Nov 13, 2024
@zaingz zaingz force-pushed the events-rule-config-optin branch 3 times, most recently from 73184cd to 56739d5 Compare November 13, 2024 15:27
@joe4dev
Copy link
Member
joe4dev commented Nov 13, 2024

test_firehose_kinesis_to_s3 looks like a flake

@zaingz zaingz force-pushed the events-rule-config-optin branch 3 times, most recently from a665b98 to 209be7b Compare November 14, 2024 21:15
joe4dev
joe4dev reviewed Nov 15, 2024
View reviewed changes
Copy link
Member
@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Could you please rebase to resolve the conflicts with master? Thank you 🙏

localstack-core/localstack/services/events/provider.py Outdated
@@ -1440,7 +1440,7 @@ def _process_rules(
LOG.info(
json.dumps(
{
"InfoCode": "InternalInfoEvents at matches_rule",
"InfoCode": "InternalInfoEvents at matches_event",
Copy link
Member

Choose a reason for hiding this comment

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

To avoid regressions in the EventStudio project, I think we should avoid this change.
Ultimately, there should be comments for every instrumentation and/or a list of instrumented places because it's currently impossible to keep track of them.

/cc @maxhoheiser @dominikschubert

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 got updated because of find and replace. but yeah it is annoying

@zaingz zaingz force-pushed the events-rule-config-optin branch from 209be7b to 87c59b8 Compare November 15, 2024 15:50
joe4dev
joe4dev reviewed Nov 15, 2024
View reviewed changes
Copy link
Member
@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

LGTM after moving the restored utils file and fixing the test skip conditional 👏

Let's wait for a full 🟢 test run of localstack-ext (see link in Notion ticket), then we're good to merge 🚀

tests/aws/services/events/test_archive_and_replay.py Outdated
@@ -362,6 +363,10 @@ def test_delete_archive_error_unknown_archive(self, aws_client, snapshot):
class TestReplay:
@markers.aws.validated
@pytest.mark.skipif(is_old_provider(), reason="not supported by the old provider")
@pytest.mark.skipif(
condition=is_v2_provider(),
Copy link
Member

Choose a reason for hiding this comment

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

This condition should be on the EVENT_RULER config because now the test gets always skipped

Copy link
Member

Choose a reason for hiding this comment

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

ideally, this should be:

    @pytest.mark.skipif(
        condition=config.EVENT_RULE_ENGINE == "python",
        reason="Not supported with Python-based rule engine",
    )

Copy link
Member

Choose a reason for hiding this comment

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

(the reason currently contradicts the condition ;)

localstack-core/localstack/services/lambda_/event_source_listeners/utils.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can we move utils.py to the event_source_mapping directory? I think it's cleaner than re-introducing the legacy directory

@thrau thrau removed their request for review November 17, 2024 13:07
joe4dev
joe4dev approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member
@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Ideally, fixing the test skip could clarify the motivation why we had to do so. At least, doing so in a follow up would be nice :)

tests/aws/services/events/test_archive_and_replay.py Outdated
@@ -362,6 +363,10 @@ def test_delete_archive_error_unknown_archive(self, aws_client, snapshot):
class TestReplay:
@markers.aws.validated
@pytest.mark.skipif(is_old_provider(), reason="not supported by the old provider")
@pytest.mark.skipif(
condition=is_v2_provider(),
Copy link
Member

Choose a reason for hiding this comment

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

ideally, this should be:

    @pytest.mark.skipif(
        condition=config.EVENT_RULE_ENGINE == "python",
        reason="Not supported with Python-based rule engine",
    )

tests/unit/utils/test_event_matcher.py

def test_matches_event_with_java_engine_strings(event_rule_engine):
"""Test Java engine with string inputs (EventBridge case)"""
event_rule_engine("java")
Copy link
Member

Choose a reason for hiding this comment

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

@dominikschubert @dfangl Should we avoid any JPype-starting monkey-patched tests in the LS CI?
I'm somewhat worried this will blow up once we combine it with other usage (e.g., SFN tests)?

This shouldn't block the merge here. I'm happy to tackle this in a follow-up because we have several other usages.

tests/aws/services/events/test_archive_and_replay.py Outdated
@@ -362,6 +363,10 @@ def test_delete_archive_error_unknown_archive(self, aws_client, snapshot):
class TestReplay:
@markers.aws.validated
@pytest.mark.skipif(is_old_provider(), reason="not supported by the old provider")
@pytest.mark.skipif(
condition=is_v2_provider(),
Copy link
Member

Choose a reason for hiding this comment

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

(the reason currently contradicts the condition ;)

@zaingz zaingz force-pushed the events-rule-config-optin branch from 87c59b8 to fd34e22 Compare November 18, 2024 13:18
@zaingz zaingz merged commit 16b0b20 into master Nov 19, 2024
31 checks passed
@zaingz zaingz deleted the events-rule-config-optin branch November 19, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@dfangl dfangl Awaiting requested review from dfangl dfangl is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

@maxhoheiser maxhoheiser Awaiting requested review from maxhoheiser maxhoheiser is a code owner

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni

@giograno giograno Awaiting requested review from giograno

@alexrashed alexrashed Awaiting requested review from alexrashed

@HarshCasper HarshCasper Awaiting requested review from HarshCasper

Assignees

@zaingz zaingz

Labels
aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@zaingz @joe4dev
0