Skip to content

Conversation

@slinkydeveloper
Copy link
Contributor

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Proposed Changes

  • Adds an example using cloudevents rust sdk to receive events

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 19, 2020
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 19, 2020
@slinkydeveloper
Copy link
Contributor Author

/hold I still need to release the sdk 😄

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2020
@slinkydeveloper
Copy link
Contributor Author

/assign @mattmoor

@mattmoor
Copy link
Member

Nice!

To build the binary, run:

```shell
cargo build --target x86_64-unknown-linux-musl --release
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 do this in a multi-stage Dockerfile as the others?

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 would prefer to avoid it here, it's quite complex and rust users don't always prefer this approach since they basically lose all the local compilation caching

Copy link
Member

Choose a reason for hiding this comment

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

I thought clearing the local compilation caching was sort of the point -- any local compiler hijinks would be ironed out and the result would be reproducible by building in Docker.

I'd also expect that Rustaceans who were familiar with both Rust and containers would probably write their own Dockerfile based on the reference, anyway.

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper May 23, 2020

Choose a reason for hiding this comment

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

Compilation caches in rust in theory are reproducible because they're per exact crate versions. What usually matters to rust users is that you don't want to clean the compilation cache, 10 mins every time for building an image is not nice 😄 Experienced rust devs usually have a remote hosted compilation cache, like this one https://github.com/mozilla/sccache, that then they use in their docker file builds

I'd also expect that Rustaceans who were familiar with both Rust and containers would probably write their own Dockerfile based on the reference, anyway.

I think so, they just need to create a multi stage build starting from debian rust image

@slinkydeveloper
Copy link
Contributor Author

@mattmoor any idea why the it test fails?

@mattmoor
Copy link
Member

re: tests, a lot have been failing today because (IIUC) a quay outage. Let's see if it's over.

/retest

@mattmoor
Copy link
Member

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020
@slinkydeveloper
Copy link
Contributor Author

/hold cancel

Now we can get this one in

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020
@slinkydeveloper slinkydeveloper requested a review from mattmoor May 20, 2020 12:10
@slinkydeveloper
Copy link
Contributor Author

@mattmoor ping

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

Taking a look -- a couple small questions, mostly about whether this is targeted at experienced Rust + container developers, or those who might be new to one or both tools.

To build the binary, run:

```shell
cargo build --target x86_64-unknown-linux-musl --release
Copy link
Member

Choose a reason for hiding this comment

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

I thought clearing the local compilation caching was sort of the point -- any local compiler hijinks would be ironed out and the result would be reproducible by building in Docker.

I'd also expect that Rustaceans who were familiar with both Rust and containers would probably write their own Dockerfile based on the reference, anyway.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2020
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels May 23, 2020
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels May 23, 2020
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, slinkydeveloper

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 501d165 into knative:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants