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.

Sign up for GitHub

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

Jump to bottom

Remove TLS from gRPC samples #196

Merged
merged 8 commits into from
Nov 30, 2020
Merged

Remove TLS from gRPC samples #196

merged 8 commits into from
Nov 30, 2020

Conversation

octonato
Copy link
Member

No description provided.

ignasi35
ignasi35 suggested changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@ignasi35 ignasi35 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a few comments re port fixing and missing changes on the scala sample.

grpc-example/grpc-example-java/build.sbt Show resolved Hide resolved
grpc-example/grpc-example-scala/build.sbt Show resolved Hide resolved
grpc-example/grpc-example-java/hello-proxy-impl/src/main/resources/application.conf Show resolved Hide resolved
ignasi35
ignasi35 approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@ignasi35 ignasi35 left a comment

Choose a reason for hiding this comment

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

Just a minor thing...

grpc-example/grpc-example-java/docs/src/main/paradox/index.md Outdated Show resolved Hide resolved
grpc-example/grpc-example-scala/docs/src/main/paradox/index.md Show resolved Hide resolved
grpc-example/grpc-example-scala/docs/src/main/paradox/index.md Show resolved Hide resolved
@ignasi35
Copy link
Contributor

The build failed both gRPC jobs in what seems like a dirty cache (invalid ap-cassandra.jar ZIP format)

@octonato
Copy link
Member Author

The build failed both gRPC jobs in what seems like a dirty cache (invalid ap-cassandra.jar ZIP format)

Yeah, I was looking into that, but it's one gRPC job and a Couchbase one. Surprisingly enough none of them should be using the cassandra embedded server as far I can tell. 🤷‍♂️

@ignasi35
Copy link
Contributor

Yeah, I was looking into that, but it's one gRPC job and a Couchbase one. Surprisingly enough none of them should be using the cassandra embedded server as far I can tell. 🤷‍♂️

withCassandra(false) on the tests, then?

@octonato
Copy link
Member Author

Well, Cassandra is not enabled.

And I got this other error, which is blocking this PR anyway:

java.lang.IllegalArgumentException: Creation of a gRPC client is useless. The ServiceTest.TestServer must be setup with SSL enabled.
	at com.lightbend.lagom.javadsl.testkit.grpc.AkkaGrpcClientHelpers.grpcClient(AkkaGrpcClientHelpers.java:95)
	at com.lightbend.lagom.javadsl.testkit.grpc.AkkaGrpcClientHelpers.withGrpcClient(AkkaGrpcClientHelpers.java:65)
	at com.lightbend.lagom.javadsl.testkit.grpc.AkkaGrpcClientHelpers.withGrpcClient(AkkaGrpcClientHelpers.java:42)
	at com.example.hello.impl.HelloServiceTest.lambda$shouldSayHelloUsingGrpc$89d0e00e$1(HelloServiceTest.java:33)
	at com.lightbend.lagom.javadsl.testkit.ServiceTest$.withServer(ServiceTest.scala:318)
	at com.lightbend.lagom.javadsl.testkit.ServiceTest.withServer(ServiceTest.scala)
	at com.example.hello.impl.HelloServiceTest.shouldSayHelloUsingGrpc(HelloServiceTest.java:31)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)

@octonato
Copy link
Member Author

@ignasi35, the tests are fixed now. I'm creating the clients directly without using the AkkaGrpcClientHelpers.

I'm wondering if it's worth to keep AkkaGrpcClientHelpers around. Maybe we should deprecate it in play-grpc. It's not adding that much value. Creating a client is not a big issue.

@ignasi35
Copy link
Contributor

@ignasi35, the tests are fixed now. I'm creating the clients directly without using the AkkaGrpcClientHelpers.

I'm wondering if it's worth to keep AkkaGrpcClientHelpers around. Maybe we should deprecate it in play-grpc. It's not adding that much value. Creating a client is not a big issue.

Yeah, that'll simplify things a lot. Let's move that direction.

@ignasi35 ignasi35 mentioned this pull request Nov 27, 2020
Open
@ignasi35
Copy link
Contributor

raised playframework/play-grpc#322

octonato
octonato commented Nov 30, 2020
View reviewed changes
.travis.yml
@@ -13,6 +13,7 @@ env:
global:
- TRAVIS_JDK=11
- JABBA_HOME=$HOME/.jabba
- RUN_DROP_TRAVIS_CACHES_STAGE=true
Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert this to false, but first want to have at least one green build.

Last time I created a custom build and set var to true it didn't work as expected. So forcing it now to move with this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of hardcoding the ENV_VAR we should remove this line andd set the ENV_VAR on the build settings in the Travis UI

ignasi35
ignasi35 approved these changes Nov 30, 2020
View reviewed changes
.travis.yml
@@ -13,6 +13,7 @@ env:
global:
- TRAVIS_JDK=11
- JABBA_HOME=$HOME/.jabba
- RUN_DROP_TRAVIS_CACHES_STAGE=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of hardcoding the ENV_VAR we should remove this line andd set the ENV_VAR on the build settings in the Travis UI

@octonato
Copy link
Member Author

@ignasi35, running now without the var. The var is already added to the settings (defaults to true). Once green, I will force a new build just to verify everything is expected.

And then we merge.

@octonato
Copy link
Member Author

@ignasi35, it failed again when running without the var, but after a clean build. I will just restart the build (var is in settings now).

@mergify mergify bot merged commit 2b2d1d4 into 1.6.x Nov 30, 2020
@mergify mergify bot deleted the rgc/remove-tls branch November 30, 2020 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignasi35 ignasi35 ignasi35 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@octonato @ignasi35