You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/07/05 08:35:07 UTC
[GitHub] [incubator-pekko-grpc] mdedetrich opened a new pull request, #106: Update latest pekko-http to bring in cors
mdedetrich opened a new pull request, #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106
Also removes the now unnecessary Snapshot OSS repo, removes other workarounds and updates pekko core to 1.0.0-RC3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252880889
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Also to note, `CorsSettings(ConfigFactory.load())` is semantically the same as the original `CorsSettings.defaultSettings`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich merged pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252847527
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
@lomigmegard So I am trying to get rid of the now deprecated `CorsSettings.defaultSettings`, do you think this is fine for now given that we don't want to change the API for the first release? Passing either `Config` or `ActorSystem` as a parameter to `defaultCorsSettings` while changing it to a `def` would require more significant changes.
I suspect that this API can use improvements in future versions (and it is marked as `@ApiMayChange`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252882955
##########
docs/src/main/paradox/server/grpc-web.md:
##########
@@ -37,9 +37,9 @@ gRPC-Web endpoint with basic CORS infrastructure in place. To use CORS,
you will need to add the pekko-http-cors dependency to your project:
@@dependency[sbt,Maven,Gradle] {
- group="ch.megard"
+ group="org.apache.pekko"
artifact="pekko-http-cors_2.12"
- version="0.0.0-SNAPSHOT" // Update when proper release of pekko-http-cors is made
+ version="0.0.0-4456-9a4c0fc7-SNAPSHOT" // Update when proper release of pekko-http-cors is made
Review Comment:
shouldn't we add some sort of pekkoHttpVersion variable that our paradox build can resolve?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252847527
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
@lomigmegard So I am trying to get rid of the now deprecated `CorsSettings.defaultSettings`, do you think this is fine for now given that we don't want to change the API for the first release? Passing either `Config` or `ActorSystem` as a parameter would require more significant changes.
I suspect that this API can use improvements in future versions (and it is marked as `@ApiMayChange`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252867936
##########
sbt-plugin/src/sbt-test/gen-scala-server/01-gen-basic-server/build.sbt:
##########
@@ -2,7 +2,9 @@
// https://github.com/akka/akka-grpc/pull/1279
scalaVersion := "2.12.18"
+// TODO remove these resolvers when we start using released Pekko jars
resolvers += Resolver.ApacheMavenSnapshotsRepo
+resolvers += "apache-staging".at("https://repository.apache.org/content/groups/staging/")
Review Comment:
This would require an upstream contribution to sbt, not against it but there are pros and cons to this. Our usage of staging here is temporary, once the first release of Pekko is made it will never be in the codebase again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252880889
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Also to note, `ConfigFactory.load()` is semantically the same as the original `CorsSettings.defaultSettings`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] He-Pin commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252867268
##########
runtime/src/main/scala/org/apache/pekko/grpc/javadsl/WebHandler.scala:
##########
@@ -32,8 +34,6 @@ import pekko.japi.function.{ Function => JFunction }
import pekko.stream.Materializer
import pekko.stream.javadsl.{ Keep, Sink, Source }
import pekko.util.ConstantFun
-import ch.megard.pekko.http.cors.javadsl.settings.CorsSettings
-import ch.megard.pekko.http.cors.javadsl.CorsDirectives
Review Comment:
Cool
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] lomigmegard commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "lomigmegard (via GitHub)" <gi...@apache.org>.
lomigmegard commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1254338735
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
@mdedetrich that should be fine yes, given the constraints of not changing the interface of this field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252872017
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Making this refer to the `ActorSystem`'s config will require more significant code changes, i.e. `defaultCorsSettings` will have to change to a `def` and due to the usage of implicit default paramters in https://github.com/mdedetrich/incubator-pekko-grpc/blob/0563fb1672c6ca831423d02925687c8635fd0d03/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala#L63 it will also require rewiring of code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] He-Pin commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252866634
##########
sbt-plugin/src/sbt-test/gen-scala-server/01-gen-basic-server/build.sbt:
##########
@@ -2,7 +2,9 @@
// https://github.com/akka/akka-grpc/pull/1279
scalaVersion := "2.12.18"
+// TODO remove these resolvers when we start using released Pekko jars
resolvers += Resolver.ApacheMavenSnapshotsRepo
+resolvers += "apache-staging".at("https://repository.apache.org/content/groups/staging/")
Review Comment:
would be better to have a constant and import it here, like `Resolver.ApacheMavenStagingRepo`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252901514
##########
docs/src/main/paradox/server/grpc-web.md:
##########
@@ -37,9 +37,9 @@ gRPC-Web endpoint with basic CORS infrastructure in place. To use CORS,
you will need to add the pekko-http-cors dependency to your project:
@@dependency[sbt,Maven,Gradle] {
- group="ch.megard"
+ group="org.apache.pekko"
artifact="pekko-http-cors_2.12"
- version="0.0.0-SNAPSHOT" // Update when proper release of pekko-http-cors is made
+ version="0.0.0-4456-9a4c0fc7-SNAPSHOT" // Update when proper release of pekko-http-cors is made
Review Comment:
Just pushed and I can confirm its rendering correctly, i.e.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252872017
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Making this refer to the `ActorSystem`'s config will require more significant code changes, i.e. `defaultCorsSettings` will have to change to a `def` and due to the usage of implicit default paramters in https://github.com/mdedetrich/incubator-pekko-grpc/blob/0563fb1672c6ca831423d02925687c8635fd0d03/runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala#L63 it will also require rewiring of code.
Or as you said it will need us to create a new actor system instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252867936
##########
sbt-plugin/src/sbt-test/gen-scala-server/01-gen-basic-server/build.sbt:
##########
@@ -2,7 +2,9 @@
// https://github.com/akka/akka-grpc/pull/1279
scalaVersion := "2.12.18"
+// TODO remove these resolvers when we start using released Pekko jars
resolvers += Resolver.ApacheMavenSnapshotsRepo
+resolvers += "apache-staging".at("https://repository.apache.org/content/groups/staging/")
Review Comment:
This would require an upstream contribution to sbt, not against it but there are pros and cons to this. Our usage of staging here is temporary (due to 1.0.0-RC3), once the first release of Pekko is made it will never be in the codebase again.
##########
sbt-plugin/src/sbt-test/gen-scala-server/01-gen-basic-server/build.sbt:
##########
@@ -2,7 +2,9 @@
// https://github.com/akka/akka-grpc/pull/1279
scalaVersion := "2.12.18"
+// TODO remove these resolvers when we start using released Pekko jars
resolvers += Resolver.ApacheMavenSnapshotsRepo
+resolvers += "apache-staging".at("https://repository.apache.org/content/groups/staging/")
Review Comment:
This would require an upstream contribution to sbt, not against it but there are pros and cons to this. Our usage of staging here is temporary (due to 1.0.0-RC3), once the first release of Pekko/pekko-http is made it will never be in the codebase again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1254341627
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Thanks, merging then!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252882955
##########
docs/src/main/paradox/server/grpc-web.md:
##########
@@ -37,9 +37,9 @@ gRPC-Web endpoint with basic CORS infrastructure in place. To use CORS,
you will need to add the pekko-http-cors dependency to your project:
@@dependency[sbt,Maven,Gradle] {
- group="ch.megard"
+ group="org.apache.pekko"
artifact="pekko-http-cors_2.12"
- version="0.0.0-SNAPSHOT" // Update when proper release of pekko-http-cors is made
+ version="0.0.0-4456-9a4c0fc7-SNAPSHOT" // Update when proper release of pekko-http-cors is made
Review Comment:
shouldn't this be derived by the build (sbt-dynver)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252879326
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
we want to retain good compatibility with akka-http-cors - so big refactors should wait till a later pekko-http release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] He-Pin commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252869260
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
Can this refers to actorsystem's config? or will have another config instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252895321
##########
docs/src/main/paradox/server/grpc-web.md:
##########
@@ -37,9 +37,9 @@ gRPC-Web endpoint with basic CORS infrastructure in place. To use CORS,
you will need to add the pekko-http-cors dependency to your project:
@@dependency[sbt,Maven,Gradle] {
- group="ch.megard"
+ group="org.apache.pekko"
artifact="pekko-http-cors_2.12"
- version="0.0.0-SNAPSHOT" // Update when proper release of pekko-http-cors is made
+ version="0.0.0-4456-9a4c0fc7-SNAPSHOT" // Update when proper release of pekko-http-cors is made
Review Comment:
So I just realized that the `@@dependency` directive in this project is broken, as in it doesn't even render correctly. Looking into it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252887493
##########
docs/src/main/paradox/server/grpc-web.md:
##########
@@ -37,9 +37,9 @@ gRPC-Web endpoint with basic CORS infrastructure in place. To use CORS,
you will need to add the pekko-http-cors dependency to your project:
@@dependency[sbt,Maven,Gradle] {
- group="ch.megard"
+ group="org.apache.pekko"
artifact="pekko-http-cors_2.12"
- version="0.0.0-SNAPSHOT" // Update when proper release of pekko-http-cors is made
+ version="0.0.0-4456-9a4c0fc7-SNAPSHOT" // Update when proper release of pekko-http-cors is made
Review Comment:
Nice find, gimme a sec
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] He-Pin commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252885028
##########
sbt-plugin/src/sbt-test/gen-scala-server/01-gen-basic-server/build.sbt:
##########
@@ -2,7 +2,9 @@
// https://github.com/akka/akka-grpc/pull/1279
scalaVersion := "2.12.18"
+// TODO remove these resolvers when we start using released Pekko jars
resolvers += Resolver.ApacheMavenSnapshotsRepo
+resolvers += "apache-staging".at("https://repository.apache.org/content/groups/staging/")
Review Comment:
I see! thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #106: Update latest pekko-http to bring in cors
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #106:
URL: https://github.com/apache/incubator-pekko-grpc/pull/106#discussion_r1252847527
##########
runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/WebHandler.scala:
##########
@@ -15,23 +15,24 @@ package org.apache.pekko.grpc.scaladsl
import scala.collection.immutable
import scala.concurrent.Future
+import com.typesafe.config.ConfigFactory
import org.apache.pekko
-import pekko.actor.ClassicActorSystemProvider
+import pekko.actor.{ ActorSystem, ClassicActorSystemProvider }
import pekko.annotation.ApiMayChange
+import pekko.http.cors.scaladsl.CorsDirectives.cors
+import pekko.http.cors.scaladsl.model.HttpHeaderRange
+import pekko.http.cors.scaladsl.settings.CorsSettings
import pekko.http.javadsl.{ model => jmodel }
import pekko.http.scaladsl.model.{ HttpMethods, HttpRequest, HttpResponse }
import pekko.http.scaladsl.model.headers._
import pekko.http.scaladsl.server.Route
import pekko.http.scaladsl.server.directives.MarshallingDirectives.handleWith
-import ch.megard.pekko.http.cors.scaladsl.CorsDirectives.cors
-import ch.megard.pekko.http.cors.scaladsl.model.HttpHeaderRange
-import ch.megard.pekko.http.cors.scaladsl.settings.CorsSettings
@ApiMayChange
object WebHandler {
/** Default CORS settings to use for grpc-web */
- val defaultCorsSettings: CorsSettings = CorsSettings.defaultSettings
+ val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
Review Comment:
@lomigmegard So I am trying to get rid of the now deprecated `CorsSettings.defaultSettings`, do you think this is fine for now given that we don't want to change the API for the first release?
I suspect that this API can use improvements in future versions (and it is marked as `@ApiMayChange`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org