Skip to content

Created support for Google Cloud HTTP functions #498

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

Merged
merged 16 commits into from
Aug 26, 2024

Conversation

Chingles2404
Copy link
Contributor

No description provided.

@Chingles2404 Chingles2404 changed the title Created support for Google Cloud HTTP functions using Node.js runtime Created support for Google Cloud HTTP functions Aug 7, 2024
@Chingles2404 Chingles2404 marked this pull request as ready for review August 15, 2024 03:52
build.sbt Outdated
Comment on lines 239 to 248
"org.typelevel" %%% "cats-effect" % catsEffectVersion,
"org.tpolecat" %%% "natchez-core" % natchezVersion,
"io.circe" %%% "circe-scodec" % circeVersion,
"io.circe" %%% "circe-jawn" % circeVersion,
"com.comcast" %%% "ip4s-core" % "3.6.0",
"org.scodec" %%% "scodec-bits" % "1.2.0",
"org.http4s" %%% "http4s-server" % http4sVersion,
"org.scalameta" %%% "munit-scalacheck" % munitVersion % Test,
"org.typelevel" %%% "munit-cats-effect-3" % munitCEVersion % Test,
"io.circe" %%% "circe-literal" % circeVersion % Test
Copy link
Member

Choose a reason for hiding this comment

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

Not all of these dependencies are used in our code, so we can remove some of them. For example natchez and circe.

def getParts(): ju.Map[String, HttpRequest.HttpPart] = ???
}

def googleResponse() = new HttpResponse {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to extract this to a named class:

class MockHttpResponse extends HttpResponse { /* ... */ }

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few very small things!

build.sbt Outdated
Comment on lines 245 to 248
mimaBinaryIssueFilters ++= Seq(
ProblemFilters.exclude[IncompatibleResultTypeProblem](
"feral.google-cloud.IOLambda.setupMemo")
),
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably not needed?

build.sbt Outdated
.settings(commonSettings)
.jsSettings(
libraryDependencies ++= Seq(
"io.circe" %%% "circe-scalajs" % circeVersion,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this dependency is used.

build.sbt Outdated
libraryDependencies ++= Seq(
"com.google.cloud.functions" % "functions-framework-api" % "1.1.0",
"co.fs2" %%% "fs2-io" % fs2Version,
"com.google.cloud.functions.invoker" % "java-function-invoker" % "1.3.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 dependency is "provided" by Google Cloud when we deploy the function. We only need to add it when we are testing the function locally.

Suggested change
"com.google.cloud.functions.invoker" % "java-function-invoker" % "1.3.1"
"com.google.cloud.functions.invoker" % "java-function-invoker" % "1.3.1" % Provided

Copy link
Member

Choose a reason for hiding this comment

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

Turns out, this was wrong: the functions-framework-api should be Provided according to the docs, and the invoker should be moved to the example project because whether or not its needed at runtime depends where you are running it.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

👏

@@ -53,11 +53,12 @@ ThisBuild / crossScalaVersions := Seq(Scala212, Scala3, Scala213)
val catsEffectVersion = "3.5.4"
val circeVersion = "0.14.7"
val fs2Version = "3.10.2"
val http4sVersion = "0.23.27"
val http4sVersion = "0.23.27-10-fa6e976-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to wait for http4s v0.23.28 before we can release, but we're good to merge and publish a snapshot!

@armanbilge armanbilge merged commit 371a63b into typelevel:main Aug 26, 2024
8 checks passed
@armanbilge armanbilge assigned armanbilge and unassigned armanbilge Oct 5, 2024
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.

2 participants