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

AWS CDK support for Tapir endpoints #2587

Merged
merged 64 commits into from
Feb 10, 2023
Merged

Conversation

gzhk
Copy link
Contributor

@gzhk gzhk commented Nov 23, 2022

No description provided.

@gzhk gzhk marked this pull request as ready for review December 5, 2022 14:51
@gzhk gzhk requested a review from adamw December 5, 2022 14:51
@@ -66,6 +66,19 @@ output the url of the created API Gateway which you can call followed by `/api/h

To destroy all the created resources run `terraform destroy`.

### CDK
Copy link
Member

Choose a reason for hiding this comment

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

We have two distinct deployment options for tapir-serverless on AWS: SAM and CDK, so I think we'd need some guide at the top as to what are the responsibilities of the various modules, and in which combinations they should be used, and which dependencies to add. Right now I think it would be challenging to a new user to understand how a serverless tapir endpoint can be deployed.

Copy link
Member

Choose a reason for hiding this comment

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

As a side note, Examples > SAM and Examples > SAM Example are confusing ;) (not directly connected to CDK, but might be good to cleanup up as well)

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 changed documentation, feel free to check whether it makes sense :)

case class AwsRequestContext(domainName: Option[String], http: AwsHttp)
case class AwsHttp(method: String, path: String, protocol: String, sourceIp: String, userAgent: String)

case class AwsResponse(cookies: List[String], isBase64Encoded: Boolean, statusCode: Int, headers: Map[String, String], body: String)
case class AwsResponse(isBase64Encoded: Boolean, statusCode: Int, headers: Map[String, String], body: String)
Copy link
Member

Choose a reason for hiding this comment

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

maybe this has been explained before, but in the 2.0 model, there are cookies - why are we removing them here?

some comment in code would be necessary if we divert from the aws model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ayeo do you remember why? 😉

Copy link

Choose a reason for hiding this comment

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

afaik headers are responsible for curring cookies. And there has never been a such a concept in AWS Response - but not sure today, needs to be checked :)

Copy link

Choose a reason for hiding this comment

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

}

/**
* I could NOT managed to enforce CDK to use payload version 2.0 At time of this writing no valuable
Copy link
Member

Choose a reason for hiding this comment

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

[minor] grammar + let's just write, that we need the v1 version to implement CDK, as that's what the framework supports

Copy link
Contributor Author

@gzhk gzhk Dec 7, 2022

Choose a reason for hiding this comment

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

I changed this one as well

package object lambda {

trait Mapper[A] {
Copy link
Member

Choose a reason for hiding this comment

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

instead of mappers and implicits, let's just add this as a method to AwsRequestV1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdyt about 3b88efa ?
As I understand the situation, at some point we would like to drop the AwsRequestV1 and use just the other one?

.use(content => Sync[F].delay(content.getLines().mkString(System.lineSeparator())))

private def copy(from: String, to: String): F[Unit] = {
val bytes = Source.fromInputStream(getClass.getResourceAsStream(sourceDir + "/" + from)).getLines().mkString("\n")
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 also be surrounded by Sync[F].blocking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,75 @@
package sttp.tapir.serverless.aws.cdk.core
Copy link
Member

Choose a reason for hiding this comment

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

[naming] I think in tapir mostly internal stuff is in an internal package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied 👍🏼

@adamw adamw mentioned this pull request Dec 16, 2022
@gzhk gzhk requested a review from adamw February 5, 2023 12:40
@adamw
Copy link
Member

adamw commented Feb 10, 2023

Great docs, thanks! :)

@adamw adamw merged commit d27d295 into softwaremill:master Feb 10, 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