-
Notifications
You must be signed in to change notification settings - Fork 360
Eventbridge Event Processor #704
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
Eventbridge Event Processor #704
Conversation
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 minor polish feedback, otherwise looks good to me.
|
||
#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] | ||
#[serde(rename_all = "kebab-case")] | ||
pub struct EventBridgeEvent { |
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.
Could this be an alias for EventBridgeEventObj<Option<String>>
to avoid code duplication?
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.
We are having some problems creating this Alias due to the use of this setting in the "detail" field:
#[serde_as(as = "serde_with::json::JsonString")]
#[serde(bound(deserialize = "T: DeserializeOwned"))]
pub detail: T,
As the input of this field will always be a String, we expect it to be a JsonString as an EventBridgeEventObj, but if you create an alias EventBridgeEventObj<Option> and its contents are a simple String, not a JsonString, the test will fail.
We're still working on solutions to avoid duplication, but so far we've had no success. Any suggestions?
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.
I'm ok with this duplication. We can change it later if we find a way.
Thanks a lot for opening this PR! I just merged some fixes for tests that were failing and were unrelated to your changes. Can you rebase from the main branch and resolve the conflicts? |
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.
Just one thing. Can you add this new feature here? https://github.com/awslabs/aws-lambda-rust-runtime/blob/main/Makefile#L66
Those tests are used to ensure that features still work in isolation. We used to have problems where some features had dependencies that were only enabled when you ran with all features enabled.
Issue #, if available:
Description of changes:
New event created to support Eventbridge.
By submitting this pull request