You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@bahir.apache.org by ire7715 <gi...@git.apache.org> on 2017/07/14 07:54:48 UTC
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
GitHub user ire7715 opened a pull request:
https://github.com/apache/bahir/pull/48
[BAHIR-122] [PubSub] Make "ServiceAccountCredentials" really broadcastable
# Make "ServiceAccountCredentials" really broadcastable
## Issue
The origin implementation broadcast the key file path to Spark cluster, then the executor read key file with the broadcasted path. Which is absurd, if you are using a shared Spark cluster in a group, you certainly not want to (and have no right to) put your key file on each instance of the cluster.
## Solution
Instead of broadcasting the key file path onto the cluster, I read the key file content in the driver node and store the binary in the `ServiceAccountCredentials`. Whenever the provider is called, it retrieve the credential with the in-memory key file.
The MetadataServiceAccount shall read the credential on the local instance, since its origin purpose is for GCE instances.
## Implementation
1. Read the `BinaryArray` into `ServiceAccountCredentials.fileBytes`
2. Determine which kind of key file to use, and create Credential. (Refer to: [com.google.cloud.hadoop.util.CredentialConfiguration.java](https://github.com/GoogleCloudPlatform/bigdata-interop/blob/master/util/src/main/java/com/google/cloud/hadoop/util/CredentialConfiguration.java#L64) and [com.google.cloud.hadoop.util.CredentialFactory.java](https://github.com/GoogleCloudPlatform/bigdata-interop/blob/master/util/src/main/java/com/google/cloud/hadoop/util/CredentialFactory.java#L225))
## Test Case
1. Introduced two key files (.json, .p12) in package resource.
2.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ire7715/bahir feature/20170713_Ire_broadcastable_credential
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/bahir/pull/48.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #48
----
commit d42d476a35d91eddeb57760eca197383edccc419
Author: ire_sun <ir...@hotmail.com>
Date: 2017-07-13T06:47:21Z
broadcast keyfile content so the credential provider need no keyfile
commit b666870a6b6fcf9166413a834c4b059ea491829a
Author: ire_sun <ir...@hotmail.com>
Date: 2017-07-14T07:18:49Z
update test cases and introduce testing credential
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on a diff in the pull request:
https://github.com/apache/bahir/pull/48#discussion_r128861101
--- Diff: streaming-pubsub/src/test/resources/org/apache/spark/streaming/pubusb/key-file.json ---
@@ -0,0 +1,12 @@
+{
+ "type": "service_account",
+ "project_id": "apache-bahir-streaming-pubusb",
+ "private_key_id": "**********this-is-fake-key-id***********",
+ "private_key": "-----BEGIN PRIVATE KEY-----\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQD6c9MDG3gq3d+3\nV6AqayUNWlC/T5Qrd3YJOItNgDxZ0bAl9FakePrivateKey1dX44uR4FomugRX3s\nENwGRcEndczGcGivTfFEB8ZeEokBQWfuWoQkJXSPaJ1rYca3l//caWxBJ+DqBw67\nF9vJqyJ23Z/kFtQOdB3+5AwfJ0b8Jq5mkQF9FL6843mHjep2LhVTcKbjJBz0K+cS\nUDr4MEoxsc0jvIDf3EwbeGWPayRzB6d558eVa+OrcCKpTxGvBJmhzsI2Ol2EcypA\nIDOFZ7OkobWdxDYhM9vUCPUNKmMs0doR9Hola8XO92D2Y4q9BoCuU+hoDPEVVQOd\nOlKCuernAgMBAAECggEAe1/rJrC1dYhu2EZWJA875WQEOvncp7zlbI1qMfdlw2lE\nOK5gmcF3zIbhuKefsH38e7zVSTlFg2I4Mb3sZTqfd+zTvz1IlHL00upxkY3X58Js\njEISriu1S5/hTDCST4aVB+L27PHUHfT0EL4kCyg+hgeO6DFGrQgObq2wOviCQ1th\nPZGccIrvAXMwGA+6OaUpnPpBbXnZKarYTGLGjoVD2eLPx+viLRKl2AW9PChdkk/0\nZvHeL7bxbYHyktK8Vp4gHStBV421HkRNlvt5S31ju75P9ReHsxCpLt1OnhBHa/gD\nimlm5fWrSHoFHx2Q+zYVt/BhmWH/Dzq1Rd+e5/vkwQKBgQD9fj6SpOYnrgNLXa25\ny4p0VHXAHweH3fpqfsSJLLuc0TWDhEtrDhVTmX35N1J5J7GIWxMGFiQxlkj+6vm6\ncOfLSUYO++HOhWdIvNzRBUJ1NSa5oIfJITAH9vPYvmrdmr5+CNZAM1KsmV7CRhvJ\nScMTVjV0gqSFKEr3QKyCLw
h3PwKBgQD87eHAwZp34DNNYWqPTb2Um9xegWnT5KYh\ntX3nxPRzyGfpPYeGedjWOwb5ST1KT0HlNhAPev02J6ZUhTrMjwHCnZcUlNiqDWdT\nlACNO810B98fO7GejjTEa6MqfaMG2m4UDA93hDBeuCOhHzXVfXvxLpUx0ABJR5Tg\nTMhkQ+AKWQKBgHYAysghEzLtgoMW/MQ8yBsXJillSHArGWNx17OzqzJ5AVxTvXf8\nelkMXuQgqLfVjoNXQifXLsoWl6xzXgU4ge7UEVTwVFF7MHVf1btHo4REVd6bqBos\n5NsQTrtbCQxX+M1a98GzIo1OaBov4Md3GuRpgUDXgBashxlKdgO0OVCpAoGANra6\n7Di1UpNEZcvaAk/938TroeH646SFr6sUJmv7uYQzvkfaJmP7XTR9qLWINaf5iDzu\nsnqXhfyDxargclnJNrFiekhMqlSl8nWEvQifxCbjxFzkank2vvrN3CY7ewMLZvjI\n68FFuem5g2Q+AAXaJu09xv3I4hFDClZxzkeY01kCgYEAv9a4vgpvGMHnjMEfq3Ym\ncbQIFq1l3djh4YqOy92EM0xr3nb1DEIvMshfhby5rwhejZ8j8m/lt/5t6uHd90/y\n60UcuPgJa2MgnPIIOZyQGH3C88o25WF9yvUAItbUtl9fxgJYdi/d9Hj821sZbhmF\nyZltoUeUMYMS4QW2OM6Dydk=\n-----END PRIVATE KEY-----\n",
+ "client_email": "pubsub-subscriber@apache-bahir-streaming-pubsub.iam.gserviceaccount.com",
--- End diff --
are there no risks with making this `key-file` public?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/97/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
Thanks @ire7715 for your fixes.
Re: key file ([comment, July 21](https://github.com/apache/bahir/pull/48#discussion_r128909173))
> **ckadner:** are there no risks with making this key-file public?
> **ire7715 :** Yes, it is okay. The key was generated as a dummy IAM service account, which now have been removed. And I have interchanged part of the private key bytes, which makes it unusable.
So, the key file is unusable for the unit test runs? If so, then there would be no reason to adding it as a test resource, no?
Is the idea then to communicate to developers how/where to add a key file they would have to generate for themselves? Would it be better then to have the unit test display a warning message in the console output if the key file is missing and skip the impacted test case(s)?
For the Jenkins CI server, we would have to install a key file that does work (keeps working) and in a pre-build step copy it from somewhere, or use an environment variable to point to a local directory that has the key file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on a diff in the pull request:
https://github.com/apache/bahir/pull/48#discussion_r128909173
--- Diff: streaming-pubsub/src/test/resources/org/apache/spark/streaming/pubusb/key-file.json ---
@@ -0,0 +1,12 @@
+{
+ "type": "service_account",
+ "project_id": "apache-bahir-streaming-pubusb",
+ "private_key_id": "**********this-is-fake-key-id***********",
+ "private_key": "-----BEGIN PRIVATE KEY-----\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQD6c9MDG3gq3d+3\nV6AqayUNWlC/T5Qrd3YJOItNgDxZ0bAl9FakePrivateKey1dX44uR4FomugRX3s\nENwGRcEndczGcGivTfFEB8ZeEokBQWfuWoQkJXSPaJ1rYca3l//caWxBJ+DqBw67\nF9vJqyJ23Z/kFtQOdB3+5AwfJ0b8Jq5mkQF9FL6843mHjep2LhVTcKbjJBz0K+cS\nUDr4MEoxsc0jvIDf3EwbeGWPayRzB6d558eVa+OrcCKpTxGvBJmhzsI2Ol2EcypA\nIDOFZ7OkobWdxDYhM9vUCPUNKmMs0doR9Hola8XO92D2Y4q9BoCuU+hoDPEVVQOd\nOlKCuernAgMBAAECggEAe1/rJrC1dYhu2EZWJA875WQEOvncp7zlbI1qMfdlw2lE\nOK5gmcF3zIbhuKefsH38e7zVSTlFg2I4Mb3sZTqfd+zTvz1IlHL00upxkY3X58Js\njEISriu1S5/hTDCST4aVB+L27PHUHfT0EL4kCyg+hgeO6DFGrQgObq2wOviCQ1th\nPZGccIrvAXMwGA+6OaUpnPpBbXnZKarYTGLGjoVD2eLPx+viLRKl2AW9PChdkk/0\nZvHeL7bxbYHyktK8Vp4gHStBV421HkRNlvt5S31ju75P9ReHsxCpLt1OnhBHa/gD\nimlm5fWrSHoFHx2Q+zYVt/BhmWH/Dzq1Rd+e5/vkwQKBgQD9fj6SpOYnrgNLXa25\ny4p0VHXAHweH3fpqfsSJLLuc0TWDhEtrDhVTmX35N1J5J7GIWxMGFiQxlkj+6vm6\ncOfLSUYO++HOhWdIvNzRBUJ1NSa5oIfJITAH9vPYvmrdmr5+CNZAM1KsmV7CRhvJ\nScMTVjV0gqSFKEr3QKyCLw
h3PwKBgQD87eHAwZp34DNNYWqPTb2Um9xegWnT5KYh\ntX3nxPRzyGfpPYeGedjWOwb5ST1KT0HlNhAPev02J6ZUhTrMjwHCnZcUlNiqDWdT\nlACNO810B98fO7GejjTEa6MqfaMG2m4UDA93hDBeuCOhHzXVfXvxLpUx0ABJR5Tg\nTMhkQ+AKWQKBgHYAysghEzLtgoMW/MQ8yBsXJillSHArGWNx17OzqzJ5AVxTvXf8\nelkMXuQgqLfVjoNXQifXLsoWl6xzXgU4ge7UEVTwVFF7MHVf1btHo4REVd6bqBos\n5NsQTrtbCQxX+M1a98GzIo1OaBov4Md3GuRpgUDXgBashxlKdgO0OVCpAoGANra6\n7Di1UpNEZcvaAk/938TroeH646SFr6sUJmv7uYQzvkfaJmP7XTR9qLWINaf5iDzu\nsnqXhfyDxargclnJNrFiekhMqlSl8nWEvQifxCbjxFzkank2vvrN3CY7ewMLZvjI\n68FFuem5g2Q+AAXaJu09xv3I4hFDClZxzkeY01kCgYEAv9a4vgpvGMHnjMEfq3Ym\ncbQIFq1l3djh4YqOy92EM0xr3nb1DEIvMshfhby5rwhejZ8j8m/lt/5t6uHd90/y\n60UcuPgJa2MgnPIIOZyQGH3C88o25WF9yvUAItbUtl9fxgJYdi/d9Hj821sZbhmF\nyZltoUeUMYMS4QW2OM6Dydk=\n-----END PRIVATE KEY-----\n",
+ "client_email": "pubsub-subscriber@apache-bahir-streaming-pubsub.iam.gserviceaccount.com",
--- End diff --
Yes, it is okay. The key was generated as a dummy IAM service account, which now have been removed. And I have interchanged part of the private key bytes, which makes it unusable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
thanks @ire7715 -- I have a few remarks regarding your latest comment:
---
> Don't know if the force push would bother you when reviewing
Thanks for not force-pushing :+1: -- It's preferable to have multiple commits in response to PR review comments and change requests. This makes it much easier to come back later to see how code changes came about. Bahir committers will squash all commits when merging Pull Requests.
So, please push another "normal" commit with your latest changes.
---
> `SparkGCPCredentialsBuilderSuite` ... ignores the test cases if the key files or email account [environment variables] are not set (or file doesn't exist) and shows the hint message
I agree mostly. We should ignore the test cases if env variables are not set. However, if the environment variables **are set** and the key file **path is invalid** then that should be an **error**. Otherwise we may not catch problems if there are changes in the Jenkins CI server.
Could you generate a set of (permanent) key files which we can integrate into our Jenkins PR builder?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/83/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on the issue:
https://github.com/apache/bahir/pull/48
@ckadner Thanks for suggestion! 😄
Just added the paragraph to instruct how to generate a proper service account and set environment variables for test cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build failed, see build log for details
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/85/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/92/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build failed, see build log for details
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
I removed the `util-hadoop` dependency and merged this PR (@bchen-talend)
@ire7715 -- Thanks for your PR!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on a diff in the pull request:
https://github.com/apache/bahir/pull/48#discussion_r128909248
--- Diff: streaming-pubsub/src/main/scala/org/apache/spark/streaming/pubsub/SparkGCPCredentials.scala ---
@@ -17,10 +17,13 @@
package org.apache.spark.streaming.pubsub
+import com.google.api.client.json.jackson.JacksonFactory
--- End diff --
Ahhh, sorry. I have added `com.google.http-client:google-http-client-jackson:1.22.0` into pom.xml.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
thanks @ire7715 -- I have a few remarks regarding your latest comment:
---
> Don't know if the force push would bother you when reviewing
Thanks for not force-pushing :+1: -- It's preferable to have multiple commits in response to PR review comments and change requests. This makes it much easier to come back later to see how code changes came about. Bahir committers will squash all commits when merging Pull Requests.
So, please push another "normal" commit with your latest changes.
---
> `SparkGCPCredentialsBuilderSuite` ... ignores the test cases if the key files or email account [environment variables] are not set (or file doesn't exist) and shows the hint message
I agree mostly. We should ignore the test cases if env variables are not set. However, if the environment variables **are set** and the key file **path is invalid** then that should be an **error**. Otherwise we may not catch problems if there are changes in the Jenkins CI server.
Could you generate a set of (permanent) key files which we can integrate into our Jenkins PR builder?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by bchen-talend <gi...@git.apache.org>.
Github user bchen-talend commented on the issue:
https://github.com/apache/bahir/pull/48
Thanks for your PR @ire7715, LGTM!
would you mind to remove this dependency, as it no use anymore.
```
<dependency>
<groupId>com.google.cloud.bigdataoss</groupId>
<artifactId>util-hadoop</artifactId>
<version>1.6.0-hadoop2</version>
</dependency>
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on the issue:
https://github.com/apache/bahir/pull/48
Thanks @ckadner for reviewing 😄
### Re: not-force pushing
Since it is preferable to push changes as other commit, and the PR will be squashed when merging. I had recovered the origin commits, and made the last changes as new commits (including [f538342](https://github.com/apache/bahir/pull/48/commits/f5383429de100c3edbb4ccd58602f3d088fceed8), [a5fd1a3](https://github.com/apache/bahir/pull/48/commits/a5fd1a314b5eec6313f2aa42648849536959db0f) and [efc0cdd](https://github.com/apache/bahir/pull/48/commits/efc0cdde76edfb11ab595ce0098a159612ef35dc)).
### Re: Invalid paths suppose to be an error not an ignorant
You are right, didn't think about it. I made the test case do not check if the file existence anymore, and instead let the `ServiceAccountCredentials` check the file existence at construction. If the file is not existed, just raise a FileNotFoundException.
### Generate a key file for Jenkins PR builder
I would love to do it! But it might be a little absurd. Here is my points:
1. There is `org.apache.spark.streaming.pubsub.PubsubTestUtils` which already uses environment variables to get the GCP project and json/p12 key files. I guess there is a set of key files are set on the Jenkins server already? Or the origin PubsubTestUtils didn't turned on by the environment variable `ENABLE_PUBSUB_TESTS`?
2. I might be the wrong person to generate the key files. Since I am not a member in the project, if I generated the key files today, and somehow someday I accidentally deleted the key file (or my Google account just got banned). The key would be no use at all, and it could be hard for you to contact me. Maybe there is a Google account for this Jenkins server, that you may generate the key file with it?
If I misunderstand you or there is anything wrong please tell me.
#### To generate a service account key file
1. Go to [Google API Console](console.developers.google.com)
2. Choose the `Credentials` Tab> `Create credentials` button> `Service account key`
3. Fill the form to create one. I did twice, one for JSON key file, another for P12.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/94/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build failed, see build log for details
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/93/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/84/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/bahir/pull/48
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/96/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
@bchen-talend -- can you take a look at this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/95/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/82/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
@ire7715 -- I create a [Google API Service account](https://console.developers.google.com/iam-admin/serviceaccounts/project?project=apache-bahir-pubsub) and [added the generated key files](https://support.cloudbees.com/hc/en-us/articles/203802500-Injecting-Secrets-into-Jenkins-Build-Jobs) to our Jenkins server. All your tests appear to be [enabled and complete successfully](http://169.45.79.58:8080/job/bahir_spark_pr_builder/95/) now.
```
[INFO] --- scalatest-maven-plugin:1.0:test (test) @ spark-streaming-pubsub_2.11 ---
Discovery starting.
Google Pub/Sub tests that actually send data has been enabled by setting the environment
variable ENABLE_PUBSUB_TESTS to 1.
This will create Pub/Sub Topics and Subscriptions in Google cloud platform.
Please be aware that this may incur some Google cloud costs.
Set the environment variable GCP_TEST_PROJECT_ID to the desired project.
Discovery completed in 135 milliseconds.
Run starting. Expected test count is: 10
SparkGCPCredentialsBuilderSuite:
- should build application default
- should build json service account
- should provide json creds
- should build p12 service account
- should provide p12 creds
- should build metadata service account
- SparkGCPCredentials classes should be serializable
Using project apache-bahir-pubsub for creating Pub/Sub topic and subscription for tests.
PubsubStreamSuite:
- PubsubUtils API
- pubsub input stream
- pubsub input stream, create pubsub
Run completed in 14 seconds, 143 milliseconds.
Total number of tests run: 10
Suites: completed 3, aborted 0
Tests: succeeded 10, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
```
---
Would you **please add a short paragraph** to the [PubSub README](https://github.com/apache/bahir/blob/master/streaming-pubsub/README.md) describing how to enable your unit tests by setting the environment variables (and how to set up a Google API *service account*, generate *key files* and how to minimally configure the *Roles* like "Pub/Sub Publisher", etc)? i.e.:
```Bash
mvn clean package -DskipTests -pl streaming-pubsub
export ENABLE_PUBSUB_TESTS=1
export GCP_TEST_ACCOUNT="apache-bahir-streaming-pubsub@apache-bahir-pubsub.iam.gserviceaccount.com"
export GCP_TEST_PROJECT_ID="apache-bahir-pubsub"
export GCP_TEST_JSON_KEY_PATH=/path/to/pubsub/credential/files/Apache-Bahir-PubSub-1234abcd.json
export GCP_TEST_P12_KEY_PATH=/path/to/pubsub/credential/files/Apache-Bahir-PubSub-5678efgh.p12
mvn test -pl streaming-pubsub
```
**Thank you!**
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/98/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on the issue:
https://github.com/apache/bahir/pull/48
Very new to the open source licensing, didn't know RAT. 😅 Glad that I know it now.
I have made all key files ends with `.key` and let RAT exclude them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Refer to this link for build results (access rights to CI server needed):
http://169.45.79.58:8080/job/bahir_spark_pr_builder/91/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build failed, see build log for details
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ire7715 <gi...@git.apache.org>.
Github user ire7715 commented on the issue:
https://github.com/apache/bahir/pull/48
Thanks @ckadner for suggestion.
Agree with your point, just modified the `SparkGCPCredentialsBuilderSuite` and make it read the environment variables for keyfiles defined in `PubsubTestUtils`. It ignores the test cases if the keyfiles or email account are not set(or file doesn't exist) and shows the hint message.
Also discovered that `ServiceAccountCredentials.getFileBuffer` didn't check file existence before opening, just added the check.
Don't know if the force push would bother you when reviewing, so I made some manual-diff:
----
SparkGCPCredentials.scala L66
\- `if (filePath.isEmpty) Array[Byte]()`
\+ `if (filePath.isEmpty || !Files.exists(Paths.get(filePath.get))) Array[Byte]()`
----
SparkGCPCredentialsBuilderSuite.scala
> Deleted resource variables, let `jsonFilePath`, `p12FilePath` and `emailAccount` read system environment.
> Implemented `jsonAssumption` and `p12Assumption`, to check the existence of corresponding environment variables and files.
> No longer expected `credential.refreshToken` thrown an Exception, expected it could retrieve the real token.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir pull request #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on a diff in the pull request:
https://github.com/apache/bahir/pull/48#discussion_r128862044
--- Diff: streaming-pubsub/src/main/scala/org/apache/spark/streaming/pubsub/SparkGCPCredentials.scala ---
@@ -17,10 +17,13 @@
package org.apache.spark.streaming.pubsub
+import com.google.api.client.json.jackson.JacksonFactory
--- End diff --
this dependency may have to be added to `streaming-pubsub/pom.xml`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ApacheBahir <gi...@git.apache.org>.
Github user ApacheBahir commented on the issue:
https://github.com/apache/bahir/pull/48
Build successful
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] bahir issue #48: [BAHIR-122] [PubSub] Make "ServiceAccountCredentials" reall...
Posted by ckadner <gi...@git.apache.org>.
Github user ckadner commented on the issue:
https://github.com/apache/bahir/pull/48
@ire7715
```
[INFO] --- scala-maven-plugin:3.2.2:compile (scala-compile-first) @ spark-streaming-pubsub_2.11 ---
...
[INFO] Compiling 3 Scala sources to /var/lib/jenkins/workspace/bahir_spark_pr_builder/streaming-pubsub/target/scala-2.11/classes...
[ERROR] /var/lib/jenkins/workspace/bahir_spark_pr_builder/streaming-pubsub/src/main/scala/org/apache/spark/streaming/pubsub/SparkGCPCredentials.scala:20: object jackson is not a member of package com.google.api.client.json
[ERROR] import com.google.api.client.json.jackson.JacksonFactory
[ERROR] ^
[ERROR] /var/lib/jenkins/workspace/bahir_spark_pr_builder/streaming-pubsub/src/main/scala/org/apache/spark/streaming/pubsub/SparkGCPCredentials.scala:74: not found: type JacksonFactory
[ERROR] val jsonFactory = new JacksonFactory
[ERROR] ^
[ERROR] two errors found
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---