adapt lambda-http to new lambda runtime api #217

softprops · 2020-04-21T16:28:35Z

Issue #, if available:

Description of changes:

Kicking the tires on reviving lambda-http crate. This is not yet finalized but I wanted to communicate progress is being made. Basic things work with the new much more simplified lambda runtime api. Still trying to figure out what good looks like.

By submitting this pull request

I confirm that my contribution is made under the terms of the Apache 2.0 license.
I confirm that I've made a best effort attempt to update all relevant documentation.

softprops · 2020-04-21T16:33:13Z

lambda-http/src/lib.rs

-    fn run(&mut self, event: Request, ctx: Context) -> Result<R, HandlerError>;
+/// Functions serving as ALB and API Gateway REST and HTTP API handlers must conform to this type.
+///
+/// This can be viewed as a `lambda::Handler` constained to `http` crate `Request` and `Response` types


Looking for input on what constitutes agreeable here. The general idea is that I'd like to lambda_http::Handler that mentally maps to lambda::Handler but whose input and output's are http crate type based. Since those types are not (de)serializable. We can't use lambda::Handler directly. I also like the idea of have an interface that pins output types to input types, something that would come in handy for triggers with contracts that define a well defined input/output shapes.

I'd like to keep the same level of flexibility has lambda::Handler in that you can define your own or "just use a closure".

Looking for input on what constitutes agreeable here.

I think what you proposed is a the most viable approach, but I need to give this a more thorough review. The only alternative I can think of that might work doing the duck-typed dispatch that the write! macro does under the hood: https://doc.rust-lang.org/std/macro.write.html

I also like the idea of have an interface that pins output types to input types, something that would come in handy for triggers with contracts that define a well defined input/output shapes.

That's interesting, but I'm not really picturing how this might work. It's not urgent, but I'd love to hear more details.

softprops · 2020-04-21T16:35:24Z

lambda-http/src/lib.rs

+    fn call(&mut self, event: Request) -> Self::Fut;
+
+    /// Consumes this Handler into an Adapter type which implements `lambda::Hander`
+    fn to_adapter(self) -> Adapter<Self> {


This was a bit annoying but I found I couldn't just implement lambda::Handler for any lambda_http::Handler directly. I needed to add a wrapping locally defined type. http://smallcultfollowing.com/babysteps/blog/2015/01/14/little-orphan-impls/ explains why. In particular the bits about "covered" types. The local Adapter type is "covering" handler impls. Open to suggestions on alternatives

Thanks for the note. Yeah, this sits at a bad intersection of trait coherence, specialization, and what rustc would might allow. It's a pickle!

I pushed a change that makes this slightly more transparent.

softprops · 2020-04-21T16:36:13Z

lambda-http/src/response.rs

@@ -15,7 +15,7 @@ use crate::body::Body;
 /// Representation of API Gateway response
 #[derive(Serialize, Debug)]
 #[serde(rename_all = "camelCase")]
-pub(crate) struct LambdaResponse {
+pub struct LambdaResponse {


because handler exports a wrapper type, these now need to be pub

davidbarsky · 2020-04-21T19:00:46Z

lambda-http/src/lib.rs

-    fn run(&mut self, event: Request, ctx: Context) -> Result<R, HandlerError>;
+/// Functions serving as ALB and API Gateway REST and HTTP API handlers must conform to this type.
+///
+/// This can be viewed as a `lambda::Handler` constained to `http` crate `Request` and `Response` types


Looking for input on what constitutes agreeable here.

I think what you proposed is a the most viable approach, but I need to give this a more thorough review. The only alternative I can think of that might work doing the duck-typed dispatch that the write! macro does under the hood: https://doc.rust-lang.org/std/macro.write.html

I also like the idea of have an interface that pins output types to input types, something that would come in handy for triggers with contracts that define a well defined input/output shapes.

That's interesting, but I'm not really picturing how this might work. It's not urgent, but I'd love to hear more details.

davidbarsky · 2020-04-21T19:02:18Z

lambda-http/src/lib.rs

+    fn call(&mut self, event: Request) -> Self::Fut;
+
+    /// Consumes this Handler into an Adapter type which implements `lambda::Hander`
+    fn to_adapter(self) -> Adapter<Self> {


Thanks for the note. Yeah, this sits at a bad intersection of trait coherence, specialization, and what rustc would might allow. It's a pickle!

lambda-http/src/lib.rs

davidbarsky · 2020-04-21T19:05:31Z

lambda-http/src/ext.rs

@@ -1,6 +1,5 @@
 //! ALB and API Gateway extension methods for `http::Request` types

-use failure::Fail;


davidbarsky · 2020-04-21T19:07:32Z

lambda-http/src/lib.rs

-//! Enriches the `lambda_runtime` crate with [http](https://github.com/hyperium/http)
-//! types targeting ALB and API Gateway proxy events.
+//! Enriches the `lambda` crate with [http](https://github.com/hyperium/http)
+//! types targeting ALB, API Gateway REST and HTTTP API proxy events.


Suggested change

//! types targeting ALB, API Gateway REST and HTTTP API proxy events.

//! types targeting ALB, API Gateway REST and HTTP API proxy events.

Note that I don't think your changes handle the schema of the new HTTP APIs yet.

getting there...

Oops! My bad.

davidbarsky · 2020-04-21T19:15:56Z

lambda-http/src/ext.rs

-#[derive(Debug, Fail)]
+#[derive(Debug)]
 pub enum PayloadError {
    /// Returned when `application/json` bodies fail to deserialize a payload
-    #[fail(display = "failed to parse payload from application/json")]
+    //#[fail(display = "failed to parse payload from application/json")]
    Json(serde_json::Error),
    /// Returned when `application/x-www-form-urlencoded` bodies fail to deserialize a payload
-    #[fail(display = "failed to parse payload application/x-www-form-urlencoded")]
+    //#[fail(display = "failed to parse payload application/x-www-form-urlencoded")]
    WwwFormUrlEncoded(SerdeError),
 }


Few notes:

Does this implement std::error::Error? If not, do you want to implement it by hand or do you want to thiserror, whose removal would be a non-breaking change?

Mark this enum as #[non_exhaustive]?

Add a PartialEq implementation?

Currently re-evaluating what I actually need. Left this commented out as a highlighted reminder for a todo.

Gotcha, thanks!

I went impl'ing std::error::Error since at this point that only really means impl fmt::Display .

At this point this enum is exhaustive. It's an error that represents not being about to deserialize a type given a application/json or application/x-www-form-urlencoded request body

Looks like I don't get an easy derive(PartialEq) since serde_json::error::Error doesn't impl it. Let me know if you really want that.

Let me know if you want to see other changes here

I went impl'ing std::error::Error since at this point that only really means impl fmt::Display.

Sounds good!

At this point this enum is exhaustive. It's an error that represents not being about to deserialize a type given a application/json or application/x-www-form-urlencoded request body

My concern is that a non-#[non_exhaustive] enum would become a forwards compatibility hazard if API Gateway (or some other networking product) added support for non-HTTP 1.1 protocols like HTTP/2 or HTTP/3. By saying this error is exhaustive, we'd need to cut a breaking change to support a new protocol. It's not the end of the world, but if it means that customers are slightly better prepared for breaking changes, I'll weakly press for #[non_exhaustive].

Looks like I don't get an easy derive(PartialEq) since serde_json::error::Error doesn't impl it. Let me know if you really want that.

Oh, bummer. Yeah, not worth it then.

davidbarsky · 2020-04-21T19:18:09Z

lambda-http/Cargo.toml

-serde_urlencoded = "0.5"
+serde_json = "^1"
+serde_urlencoded = "0.6"
+tokio = "^0.2"


I haven't though through this yet, but—what parts of Tokio need to be pulled into lambda_http as a project dependency?

This list just got resorted. I don't think there are any actually. I'll double check and remove if so. I only needed a dev-dependency for #[tokio::main] bits but I don't think we're actually referencing tokio types directly yet which shines a light on some nice design choices for the new api. Thanks for that.

I only needed a dev-dependency for #[tokio::main] bits but I don't think we're actually referencing tokio types directly yet which shines a light on some nice design choices for the new api. Thanks for that.

Glad you like it :)

davidbarsky · 2020-04-21T19:19:19Z

Thanks so much for doing this. I posted a review/questions kicking the tires on your tire kicking :)

softprops · 2020-04-21T22:45:54Z

I posted a review/questions kicking the tires on your tire kicking

Thanks. I'll keep this in draft until I'm ready so expect more change. I'm just using the draft to communicate progress being made. It's still baking in bread in the oven.

davidbarsky · 2020-04-21T23:04:59Z

Sounds good, I'll hold off on making additional comments until this is moved out of a draft state.

softprops · 2020-04-22T05:00:25Z

lambda-http/src/request.rs

 #[serde(untagged)]
-pub enum RequestContext {
-    /// Api Gateway request context
+pub enum LambdaRequest<'a> {


Changing the strategy here. previously there was one struct made to look look like both apigw rest api proxy and alb events. With the addition of http api events that approach was starting to look long in the tooth and untenable. Going back to the drawing board on this answering "what is this even?" question. I came up with roughly

type LambdaRequest = ApiGatewayRequest | ApiGatewayV2Request | AlbRequest;

Unionizing types rather than masquerading types had some tradeoffs and some useful benefits. Updating/evolving one variant doesn't require additional consideration in how another gets deserialized. Transforming 3 similar variants incurs some boiler plate but I could add some helpers to make that less onerous. In retrospect this feels like a better approach given the tradeoffs.

lambda-http/src/request.rs

lambda-http/src/ext.rs

softprops · 2020-04-25T13:38:14Z

Ok @davidbarsky. I'm pretty happy with this now. I also tested a few deployments to check for regressions. Let me know what you think.

davidbarsky · 2020-04-29T16:05:23Z

lambda-http-attributes/Cargo.toml

@@ -0,0 +1,13 @@
+[package]
+name = "lambda-http-attributes"


Do you think this functionality could be a part of lambda-attributes? In tracing, the #[instrument] macro can instrument std::future::Future/async functions if another crate (tracing_futures) is in the dependency graph. See here: https://docs.rs/tracing/0.1.13/tracing/#spans-1.

I'd like it if we can do something similar here to reduce how many crates customers might need and how many crates we need to release.

The way this works now is that the attribute crates are transient dependencies users don't have to manually manage these.

I have some GitHub workflows for publishing crates easy as git pushing a tag for automating releases I could share with this project.

I was trying to find a way to do this with lambda attributes but couldn't find a way do to it without adding a unnatural and cyclical dependency on lambda-http in the lambda crate.

One idea I haven't excersized is compilation flags set by the lambda and lambda-http crates respectively. Thoughts on that approach?

I really like the procmacro-main pattern but it's notably a bit boilerplatey to set up which makes it seems like the yet-another-crate-to-publish feelings are strong. I can sympathize with that.

I took a look at what the tracing crate does. It seems it uses "asyncness' as a cue to generate code using tracing_futures It's not actually aware if tracing_futures is on the dependency graph or not. There's a little less luxury here.

I'll see if I can use the type as the first argument to main as a cue to generate lambda http references then fallback on the cargo feature flag if not.

I took a look at what the tracing crate does. It seems it uses "asyncness' as a cue to generate code using tracing_futures It's not actually aware if tracing_futures is on the dependency graph or not. There's a little less luxury here.

Ah yeah, I forgot how we (I?) wrote that.

I'll see if I can use the type as the first argument to main as a cue to generate lambda http references then fallback on the cargo feature flag if not.

Trying to figure out a type in a macro expansion isn't easy, least of all because I ran tried to resolve poor interactions with the async_trait and tracing's #[instrument]. You'd need something like specialization to properly distinguish between values in a macro, and short of that, autoref specialization: https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md. I'm not sure it's worth tackling now, but if you come up with solution, please, I'm all ears!

I realized that procmaco doesn't really know about types, only syntax so distinguishing a type seems less feasible. I'll try a cargo feature flag something like a feature for raw handlers and http handlers.

[features] raw = [] http = [] defaults = [raw]

[dependencies] lambda-attributes = { path = "../lambda-attributes", version = "0.1", default_features = false, features = [http] }

then do #[cfg_attr(feature = "raw")] vs #[cfg_attr(feature = "http")] checks

hit a small snag here I wanted to share early feedback on. I added the features mentioned above to lambda-attributes then in its impl

if cfg!(feature = "http") { // handler code for http events } else { // handler code for raw events }

I'm able to successfully run cargo test -p lambda and cargo test -p lambda_http separately but when running cargo test the block for lambda_http gets emitted from the proc-macro presumably because cargo pins one version of the lambda-attribute dependency with one set of features for all crates in your workspace. This seems like a bug in cargo or it could be a feature. Either way its a bit problematic for the cargo feature flag approach.

error[E0433]: failed to resolve: use of undeclared type or module `lambda_http` --> lambda/examples/hello.rs:6:1 | 6 | #[lambda] | ^^^^^^^^^ use of undeclared type or module `lambda_http`

I believe this behavior relates to this (open) cargo issue rust-lang/cargo#4463. I expect workspaces projects for rust lambdas be common with a mix of http handlers and some coordinating (sqs, s3, ect) handlers in the same project which makes me weary of the cargo feature flag approach.

I found a way out that I actually like better using item attr args that could set the stage for other strictly typed handlers

#[lambda] does what it does today

#[lambda(http)] emits http handler code

davidbarsky

Some comments; by no means exhaustive. I'll add some more other stuff.

lambda-http/Cargo.toml

davidbarsky · 2020-04-29T16:07:39Z

lambda-http/Cargo.toml

@@ -3,7 +3,7 @@ name = "lambda_http"
 version = "0.1.1"


Bump to 0.2.0-beta.1?

davidbarsky · 2020-04-29T16:08:54Z

lambda-http/examples/basic.rs

    Ok(())
 }

-fn my_handler(e: Request, c: Context) -> Result<impl IntoResponse, HandlerError> {
-    Ok(match e.query_string_parameters().get("first_name") {
+async fn func(event: Request) -> Result<impl IntoResponse, Error> {


Question—why an impl IntoResponse rather than http::Response`?

(More for customers than for me.)

Rust convenience and extensibility pattern.

For some common cases you'll likely just want to yield a string or serde json value. IntoResponse is a convince here, similar the new http apis interface for implicit json responses. In other cases where you want a more application specific way of structuring responses for consistency, you can just return your own type and impl into response to avoid the conversion inside the hander. Side effect, it makes that a bit wiring more unit testable.

In short make the simple cases easy and complex cases possible.

Concretely in this case there are two match arms one for the happy path which returns a string and one for the user error case which builds a response with an error status. The common interface for the result type becomes some type impl IntoResponse.

lambda-http/src/body.rs

davidbarsky · 2020-04-29T16:10:26Z

lambda-http/src/ext.rs

+        }
+    }
+}
+impl Error for PayloadError {}


Implement Error::source?

lambda-http/src/lib.rs

davidbarsky · 2020-04-29T16:32:52Z

lambda-http/src/lib.rs

+//! For the simple cases you may not need much if any bootstrapping. To make life simpler
+//! you can add an `#[lambda_http]` attribute to your `main` function and `lambda::run` machinery
+//! will be wired in for you.


Suggested change

//! For the simple cases you may not need much if any bootstrapping. To make life simpler

//! you can add an `#[lambda_http]` attribute to your `main` function and `lambda::run` machinery

//! will be wired in for you.

//! Adding an `#[lambda_http]` attribute to a `#[tokio::run]`-decorated `main` function will setup and run the Lambda function.

davidbarsky · 2020-04-29T16:33:49Z

lambda-http/src/request.rs

+#[derive(Deserialize, Debug, Clone)]
+#[serde(untagged)]
+pub enum RequestContext {
+    /// API Gateway v2 request context
+    ApiGatewayV2(ApiGatewayV2RequestContext),
+    /// API Gateway request context
+    ApiGateway(ApiGatewayRequestContext),
+    /// ALB request context
+    Alb(AlbRequestContext),
+}


Mark this as #[non_exhaustive].

davidbarsky · 2020-04-29T16:43:43Z

lambda-http/src/request.rs

+/// Event request context as an enumeration of request contexts
+/// for both ALB and API Gateway and HTTP API events


Suggested change

/// Event request context as an enumeration of request contexts

/// for both ALB and API Gateway and HTTP API events

/// Support invocations from Application Load Balancers and API Gateway. Supported API Gateway invocations

/// include both [REST](https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-rest-api.html) and [HTTP](https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api.html) APIs.

davidbarsky · 2020-04-29T16:44:57Z

lambda-http/tests/data/apigw_v2_proxy_request.json

@@ -0,0 +1,39 @@
+{


For later: let's create a JSON schema based off this file (https://app.quicktype.io/ is great) and fuzz/quickcheck the handler based off the schema?

Interesting you mentioned that. I was actually think about starting a fuzzer project for lambdas, a cli, similar to Sam's templated events but random and standalone. But dully noted for unit testing as well.

(note: this isn't a blocker for this PR; tracking this in an issue is fine.)

One of the reasons the simulated.rs module exists is to support this use-case without something like SAM. I haven't spoken to the SAM team about this, however.

dbasedow · 2020-05-05T19:41:56Z

I think cookies should be an Option<...> in LambdaRequest::ApiGatewayV2 in my tests the lambda event did not contain that key and i got a "data did not match any variant of untagged enum LambdaRequest" error.

Great work on the PR. It's exactly what I was looking for!

softprops · 2020-05-06T23:17:06Z

I think cookies should be an Option<...> in LambdaRequest::ApiGatewayV2 in my tests the lambda event did not contain that key and i got a "data did not match any variant of untagged enum LambdaRequest" error

Nice catch. I deployed a lambda with the v2 payload enabled and grabbed an example of an "echo" of its event payload then added a unit test to make sure the minimal case is covered in 94b1824

Co-authored-by: David Barsky <[email protected]>

softprops · 2020-05-06T23:34:18Z

rebased on master and found some changed to the Handler::Err which is now Handler::Error. Working through accommodating those

softprops · 2020-05-07T05:00:12Z

@davidbarsky I think I addressed the comments above. Let me know if there are any more blockers.

a few side question/comment which should maybe be a separate gh issue.

I've love to see the versions of theses crates keep a shared version making it easier for users to manage. It's subjective but the cost of publishing all crates when on changes is less than that of a user's mental overhead of keeping track of the combinations of versions of crates all published from the same project.

What are you're thoughts on making all versions the same in this project.

softprops · 2020-05-07T05:04:08Z

Is there any objective reason the minimum supported version in ci can't be stable? I noticed a ci failure when I introduces a matches! call introduced in 1.42

davidbarsky · 2020-05-07T14:05:33Z

@davidbarsky I think I addressed the comments above. Let me know if there are any more blockers.

No, nothing that should block merging of this PR, I think. We can capture it in followups.

I've love to see the versions of theses crates keep a shared version making it easier for users to manage. It's subjective but the cost of publishing all crates when on changes is less than that of a user's mental overhead of keeping track of the combinations of versions of crates all published from the same project.
What are your thoughts on making all versions the same in this project.

Yeah, I think I'd be in favor of publishing everything/re-exporting everything through the Lambda crate and enabling subcrates through feature flags. I'd diverge from Tokio and have everything enabled by default, though. I think a blocker to this would be to collapse the two macro crates into a single one and try to have a generic Into bound on the handler definition, if that makes sense.

Is there any objective reason the minimum supported version in ci can't be stable? I noticed a ci failure when I introduces a matches! call introduced in 1.42?

I didn't know that was a thing! Let's make our MSRV the current stable Rust release until we hit 1.0. Our versioning policy should allow for bug fix releases to rely on a recent Rust release, but we should document this in a prominent location.

davidbarsky · 2020-05-08T15:26:38Z

I'll merge this in and we catch any fallout of issues later on.

softprops commented Apr 21, 2020

View reviewed changes

davidbarsky reviewed Apr 21, 2020

View reviewed changes

softprops commented Apr 22, 2020

View reviewed changes

lambda-http/src/request.rs Show resolved Hide resolved

softprops commented Apr 22, 2020

View reviewed changes

lambda-http/src/request.rs Show resolved Hide resolved

softprops commented Apr 22, 2020

View reviewed changes

lambda-http/src/request.rs Show resolved Hide resolved

softprops mentioned this pull request Apr 23, 2020

Fails when used with httpApi gateway #212

Closed

softprops marked this pull request as ready for review April 25, 2020 02:15

softprops commented Apr 25, 2020

View reviewed changes

lambda-http/src/ext.rs Show resolved Hide resolved

softprops mentioned this pull request Apr 27, 2020

awkward HandlerError requirements #94

Closed

davidbarsky reviewed Apr 29, 2020

View reviewed changes

softprops added 10 commits May 6, 2020 19:25

adapt lambda-http to new lambda runtime api

60e222a

if all Handlers are Adapters and all Adapters are Handlers, we win

cc3286f

no direct tokio dependency needed

dc24742

progress towards alb + apigw rest + http api => http::Request

f8f65bc

unwrap sets a bad example for others. avoid it where possible

f923709

macros will do

e63c6b9

append raw query string where available

0be80f9

impl std Error for PayloadError

ad09d30

revive some commented tests

c053a9a

correct docs

bf0d0c6

softprops and others added 17 commits May 6, 2020 19:25

consistency

127f2cc

include lambda-http-attributes in base cargo workspace member list

b207bf1

Update lambda-http/Cargo.toml

c4687db

Co-authored-by: David Barsky <[email protected]>

Update lambda-http/src/body.rs

ea5327d

Co-authored-by: David Barsky <[email protected]>

Update lambda-http/src/ext.rs

2db44f4

Co-authored-by: David Barsky <[email protected]>

Update lambda-http/src/lib.rs

2435b34

Co-authored-by: David Barsky <[email protected]>

Update lambda-http/src/lib.rs

92b6217

Co-authored-by: David Barsky <[email protected]>

Update lambda-http/src/lib.rs

4fa1f9a

Co-authored-by: David Barsky <[email protected]>

bump lambda-http version

03365ff

bump version cargo.lock

a4f7823

expose std::error::Error#source for PayloadError

ec04100

prefer fully qualified names when it comes to hyperium/http crates

afd3b4c

prefer .expect(msg) in tests over tests that return results

5a5e2a6

update wording in rustdoc

aefd338

pr feedback about docs

6796154

run rustfmt

da0ef2e

add test for minimmal httpApi payload

056adea

align with master Handler api changes

95e7ccd

softprops force-pushed the revive-lambda-http branch from 94b1824 to 95e7ccd Compare May 6, 2020 23:37

softprops added 3 commits May 6, 2020 22:49

consolidate attributes crates into one

186d411

this file doesnt need to change

fb79014

use cleaner matches macro

c9dea9b

make the minimum supported rust version the latest stable release

96bf3bf

davidbarsky approved these changes May 8, 2020

View reviewed changes

davidbarsky merged commit 4463351 into awslabs:master May 8, 2020

		@@ -1,6 +1,5 @@
		//! ALB and API Gateway extension methods for `http::Request` types

		use failure::Fail;

	//! types targeting ALB, API Gateway REST and HTTTP API proxy events.
	//! types targeting ALB, API Gateway REST and HTTP API proxy events.

		/// Event request context as an enumeration of request contexts
		/// for both ALB and API Gateway and HTTP API events

adapt lambda-http to new lambda runtime api #217

adapt lambda-http to new lambda runtime api #217

Conversation

softprops commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky commented Apr 21, 2020

softprops commented Apr 21, 2020

davidbarsky commented Apr 21, 2020

Choose a reason for hiding this comment

softprops commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbasedow commented May 5, 2020

softprops commented May 6, 2020

softprops commented May 6, 2020

softprops commented May 7, 2020

softprops commented May 7, 2020

davidbarsky commented May 7, 2020

davidbarsky commented May 8, 2020

softprops commented Apr 21, 2020 •

edited

Loading