Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Eventbridge v2: Add pattern matching #10664
Feat: Eventbridge v2: Add pattern matching #10664
Changes from 26 commits
5c4a8fd
71dc9b3
99841de
10c7b4a
8dc03db
6cc9171
5360ecf
f6ebd00
b663463
bf850f2
b42c2e4
a93c0f9
4066cfd
ede757d
28354be
f053084
084c866
f86c5c6
e7fa9e1
bad5497
989becd
3a99a86
ca62eb2
55eb484
12efbac
b011d8a
81d63c0
aae673a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
style: the
event
is not needed for a time conversion. Passing minimal knowledge is considered best practice. Logging could still happen at a different place.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.
true, but in this case the function not only serves to convert time but also conditionally set the time based if the time filed is set in events or not.
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.
ok, then the naming
get_event_time
is a bit misleading here. Probably worth testing/refactoring in a follow-up.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.
Is this validated AWS behavior? Do we want a fallback here or should we fail?
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.
this is from this fix in this pr: #8690
I cannot reproduce the bug since boto validates input and throws an error if it is not a string that can be parsed to a valid datetime object. I would keep it in for now, since it is mentioned that it has been an issue.
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.
Botocore validation can be disabled using
parameter_validation=False
(see botocore docs). If it's a known issue, it might be worth adding a test case in a follow-up.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.
According to the AWS docs,
detail
should always be a JSON object. Hence, I guess we can safely assume that other strings or base64-encoded content is not expected here, right?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.
this is validated AWS behavior - see tests
test_put_events_nonexistent_event_bus
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.
super nit:
event_pattern_str
or str typing could clarify the type here (especially given we have both versions in our codebase)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.
Now for the v2 provider, since we use only the Java matching engine we only ever have event pattern as a string, we don't parse it into a JSON object. In addition, the API string type is called
EventPattern
, therefor I would keep it without the str suffix.Also, I think the naming convention of match_rule is misleading since we don't match the rule but the rule_pattern and only pass in the pattern. We should think of updating this in the
event_ruler
.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.
It's a good thing we don't need JSON parsing for the event pattern anymore in the new provider, but right one line below we have the counter-example of transforming an
event
into anevent_str
😬 . Looking over the many implementations in LocalStack, thisstr
vsjson
duality was very confusing, and being explicit can help to clarify the API usage here.How do you think does
rule
differ fromrule_pattern
?Background
The terminology
Ruler.matchesRule(event, rule)
in theevent_ruler.py
follows the official AWS event-ruler and aims to be more generic than just for EventBridge. Notice that different services use slightly different terminology:FilterCriteria
FilterCriteria
filter policy
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.
json vs strg makes sense - the old fallacy if you build it you know it and think its obvious to everyone else