feat: Add proper example of configuring Rust functions. #436
Conversation
|
@dejanb: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
size/M
|
/kind enhancement |
knative-prow-robot
added
kind/enhancement
needs-rebase
dejanb
force-pushed
the
rust-config-example
branch
from
ef91a34
to
695dda3
Compare
knative-prow-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Looks good to me. Though again, I'm not a rustafarian. Would love for @jcrossley3 to have a look.
templates/rust/events/src/handler.rs
Outdated
| @@ -29,7 +34,7 @@ mod tests { | |||
| async fn valid_input() { | |||
| let mut input = Event::default(); | |||
| input.set_data("application/json", json!({"name": "bootsy"})); | |||
| let resp = handle(input).await; | |||
| let resp = handle(input, web::Data::new(HandlerConfig::default())).await; | |||
There was a problem hiding this comment.
I really wish rust supported optional arguments. :)
Can we put this in a static constant local to the test module and call it config or something? Just so it won't look so gross?
There was a problem hiding this comment.
I ended up adding config() function to tests that wraps this and makes things a bit nicer to look at :). This is the simplest thing I could find as making static traits complicates them for the example and I would like to keep those as simple and reusable as possible. Let me know how it looks.
| pub async fn index(req: HttpRequest, config: web::Data<HandlerConfig>) -> HttpResponse { | ||
| info!("{:#?}", req); | ||
| if req.method() == Method::GET { | ||
| HttpResponse::Ok().body("Hello!\n") | ||
| HttpResponse::Ok().body(format!("Hello {}!\n", config.name)) | ||
| } else { | ||
| HttpResponse::Ok().body("Thanks!\n") | ||
| HttpResponse::Ok().body(format!("Thanks {}!\n", config.name)) |
There was a problem hiding this comment.
This kinda makes me want to add a name argument, similar to the events example, in which the config.name is only used if the name isn't passed.
There was a problem hiding this comment.
I'm all for improving this example and looked into it. However, I didn't find an easy/simple way to add it. We'd need to use extractors for both query and body (depending whether get or post is used) and possibly add another struct. Maybe we should split the handler in two? We would also need to add more tests.
I would definitely like to evolve this, but maybe we should do it in a separate PR?
There was a problem hiding this comment.
Yeah, I was thinking there was a FromRequest impl for String but that returns the whole body. I'm not sure introducing a Query param (or whatever) is worth it. Probably out of scope for an example. My bad!
dejanb
force-pushed
the
rust-config-example
branch
3 times, most recently
from
6d6be3d
to
5e617e7
Compare
|
/lgtm |
|
Thanks for helping out with the review @jcrossley3 /lgtm |
|
/approve |
|
@dejanb pkged.go is such a pita. Could you please rebase this PR? |
knative-prow-robot
added
the
needs-rebase
dejanb
force-pushed
the
rust-config-example
branch
from
5e617e7
to
a77283b
Compare
knative-prow-robot
removed
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dejanb, lance The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
/lgtm |
Changes
-:gift: As discussed in #430, here's the concrete example on how to use function configuration.
/kind feature
It uses a simple configuration struct to provide users with easy to adapt example on how to provide configuration to their functions.