fix GCP pub/sub auth issue - remore redundant apikey param #427
Conversation
andyoll
commented
Jul 27, 2017
andyoll
commented
- remove 'apikey' param from HTTP request URL for GCP Pub/Sub publish request.
…sh request #426 * remove 'apikey' param from HTTP request URL for GCP Pub/Sub publish request.
|
Hi @andyoll, Thank you for your contribution! We really value the time you've taken to put this together. We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it. |
|
CLA signed |
|
@andyoll it seems the travis build is failing because scalariform disagrees with some whitespace |
|
There remains a failing test in #1005.3 - a problem with connecting to MQTT server. |
|
@raboof - is there a means to get Travis to re-run tests for this PR (without making a dummy commit) - I'm thinking the failed test is not caused by my changes, but due to an environment problem when Travis ran.. |
|
going to close and re-open this PR to force new Travis build |
There was a problem hiding this comment.
https://groups.google.com/forum/#!topic/cloud-pubsub-discuss/8fGaG5cWiTk suggests the 'key' parameter can be useful when authenticating to something public - is that not (or no longer) the case?
| @@ -102,7 +102,7 @@ private trait HttpApi { | |||
| ): Future[immutable.Seq[String]] = { | |||
| import materializer.executionContext | |||
|
|
|||
| val url: Uri = s"$PubSubGoogleApisHost/v1/projects/$project/topics/$topic:publish?key=$apiKey" | |||
| val url: Uri = s"$PubSubGoogleApisHost/v1/projects/$project/topics/$topic:publish" | |||
There was a problem hiding this comment.
Hmm too bad we didn't have a test that touched this part of the flow - can you think of a reasonable one to add?
There was a problem hiding this comment.
Key param seems to make auth fail - whereas, with it removed auth works nicely.
Regarding tests: there is a test defined in google-cloud-pub-sub/src/test/scala/akka/stream/alpakka/googlecloud/pubsub/HttpApiSpec.scala with mocking, that I updated as part of this PR. The only means to improve this to ensure the code 'really works' (that I can think of), is to actually hit the Google API, with real credentials - this could guard against any future behaviour changes. What do you think?
|
(mqtt failure again, restarted that travis build) |
| @@ -80,7 +80,7 @@ class HttpApiSpec extends FlatSpec with BeforeAndAfterAll with ScalaFutures with | |||
| mock.register( | |||
| WireMock | |||
| .post( | |||
| urlEqualTo(s"/v1/projects/${TestCredentials.projectId}/topics/topic1:publish?key=${TestCredentials.apiKey}") | |||
| urlEqualTo(s"/v1/projects/${TestCredentials.projectId}/topics/topic1:publish") | |||
There was a problem hiding this comment.
ah I didn't realize this is a check, not a 'given' 👍
|
Thanks guys. I originally added that and can confirm the apikey is redundant. I've since learnt better. :) |
|
I agree with @smithzd. Using Pub-Sub in a |
|
I think it needs to be removed from the ack as well -- I tested pull and ack without it and it works. |
|
Hi - as someone who just ran into this, what's the status of this PR? As far as I can tell it's not just redundant it's simply not allowed, e.g. pubsub isn't one of the supported APIs for api keys |
