Skip to content
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

tide sdk and example #117

Closed
wants to merge 16 commits into from
Closed

tide sdk and example #117

wants to merge 16 commits into from

Conversation

No9
Copy link
Contributor

@No9 No9 commented Mar 25, 2021

Signed-off-by: Anthony Whalley [email protected]
This PR adds support for tide, the server framework of http-rs.
https://github.com/http-rs/tide
It uses 0.3.1 of the cloud events library.
The code follows the actix implementation but avoids using the async-trait library as it wasn't clear what the benefit of that would be and async-trait comes with a warning on performance.
I'd appreciate any feedback and hope someone can spare some time to land this.

Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Sounds good overall, I left some minor comments. Sorry for the late response

@@ -0,0 +1,28 @@
[package]
name = "cloudevents-sdk-tide"
version = "0.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

This should match the sdk version


let versionheader = match self.headers.get("ce-specversion") {
Some(s) => s.as_str(),
None => "",
Copy link
Member

Choose a reason for hiding this comment

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

No spec version should fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this @slinkydeveloper I will take a look and update

@No9
Copy link
Contributor Author

No9 commented May 8, 2021

Hey @slinkydeveloper
I've bumped the version number in the Cargo.toml as requested

I also took a look at the your spec version comment and I'm not quite sure about where the problem is.

In the code a bad version would fail at the next line
https://github.com/cloudevents/sdk-rust/pull/117/files#diff-dbbbfcfd809f12a6c475c24be0cc416d549ebfe6cd2be0009da483c1b00f08d3R36
as it calls try_from https://github.com/cloudevents/sdk-rust/blob/master/src/event/spec_version.rs#L67
and this would generate an error if it's not 0.3 or 1.0.
Or have I over looked something obvious?

I have also taken a further look at the API and made it async and enhanced the request test to include time.

No9 added 7 commits May 9, 2021 08:38
Signed-off-by: Anthony Whalley <[email protected]>
Signed-off-by: Anthony Whalley <[email protected]>
Signed-off-by: Anthony Whalley <[email protected]>
Signed-off-by: Anthony Whalley <[email protected]>
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

This PR contains changes unrelated to tide, can you move them out of this PR?

@@ -162,7 +160,7 @@ mod tests {
//TODO this is required now because the message deserializer implictly set default values
// As soon as this defaulting doesn't happen anymore, we can remove it (Issues #40/#41)
.time(time)
.source(Url::from_str("http://localhost").unwrap())
.source("http://localhost")
Copy link
Member

Choose a reason for hiding this comment

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

Can you move these changes to actix-web and reqwest in another PR?

Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Can you rebase?

Ok(())
},
)
.with_protocols(&["cloudevents.json"]),
Copy link
Member

Choose a reason for hiding this comment

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

That's cool!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look at the rebase

@slinkydeveloper
Copy link
Member

Can you rebase the library to work with the new package reorg?

@No9
Copy link
Contributor Author

No9 commented Jan 5, 2023

Closing as the requirement I was building for has been superseded by #202

@No9 No9 closed this Jan 5, 2023
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