Port Google Cloud classes to the new structure #156

rtannenbaum · 2018-02-23T21:57:57Z

holachuy · 2018-02-26T17:27:05Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleBucketStore.java

+final class GoogleBucketStore implements BucketStore {
+  private static final String APP_CREDENTIAL_BUCKET_PREFIX = "app-data-";
+
+  private Storage storage;


both of these member vars can be final

whoops, thanks. I need @seehamrun's linter!

holachuy · 2018-02-26T17:28:45Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+      throws GoogleCredentialException {
+    validateUsingGoogle(context);
+
+    String env = context.getConfiguration("environment", "LOCAL");


TODO to make a parameter?

holachuy · 2018-02-26T17:29:01Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+  public @interface ProjectId {}
+
+  @Override
+  protected void configure() {}


Do we need requireBinding(ExtensionContext.class)

Done, didn't see an open source Module dependency test to catch this next time

holachuy · 2018-02-26T17:29:13Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    validateUsingGoogle(context);
+
+    String env = context.getConfiguration("environment", "LOCAL");
+    if (env.equals("LOCAL")) { // Running locally


LOCAL should be a constant

holachuy · 2018-02-26T17:29:44Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+      // This is a crude check to make sure we are only pointing to test projects when running
+      // locally and connecting to GCP
+      Preconditions.checkArgument(
+          projectId.endsWith("-local") || projectId.endsWith("-test") || projectId.endsWith("-qa"),


project id conventions could be put in a constant or enum with explanation of the suffices.

holachuy · 2018-02-26T17:30:18Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+              + " be a local/test project since it doesn't end in -local, -test, or -qa.");
+    } else { // Assume running on GCP
+      // TODO: Check whether we are actually running on GCP once we find out how
+      String credsLocation = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");


constant for GOOGLE_APPLICATION_CREDENTIALS

holachuy · 2018-02-26T17:30:58Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    } else { // Assume running on GCP
+      // TODO: Check whether we are actually running on GCP once we find out how
+      String credsLocation = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");
+      if (!credsLocation.startsWith("/var/secrets/")) {


We could make a constant for /var/secrets, maybe something like KUBERNETES_SECRETS_PATH with documentation or link

holachuy · 2018-02-26T17:31:32Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    validateUsingGoogle(context);
+    String projectId;
+    try {
+      projectId = System.getenv("GOOGLE_PROJECT_ID");


GOOGLE_PROJECT_ID could be in a constant or enum

holachuy · 2018-02-26T17:32:30Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+   * Google cloud?
+   */
+  private void validateUsingGoogle(ExtensionContext context) {
+    if (!context.getConfiguration("cloud", "").equals("GOOGLE")) {


"cloud" should probably come from a global flag name and/or constant or enum.

"GOOGLE" should be a constant in this class

rtannenbaum

Thanks for the comments, much more readable now!

rtannenbaum · 2018-02-28T16:34:30Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleBucketStore.java

+final class GoogleBucketStore implements BucketStore {
+  private static final String APP_CREDENTIAL_BUCKET_PREFIX = "app-data-";
+
+  private Storage storage;


whoops, thanks. I need @seehamrun's linter!

rtannenbaum · 2018-02-28T16:36:06Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+      throws GoogleCredentialException {
+    validateUsingGoogle(context);
+
+    String env = context.getConfiguration("environment", "LOCAL");


rtannenbaum · 2018-02-28T16:58:53Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+      // This is a crude check to make sure we are only pointing to test projects when running
+      // locally and connecting to GCP
+      Preconditions.checkArgument(
+          projectId.endsWith("-local") || projectId.endsWith("-test") || projectId.endsWith("-qa"),


rtannenbaum · 2018-02-28T16:58:57Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    validateUsingGoogle(context);
+
+    String env = context.getConfiguration("environment", "LOCAL");
+    if (env.equals("LOCAL")) { // Running locally


rtannenbaum · 2018-02-28T17:01:29Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+  public @interface ProjectId {}
+
+  @Override
+  protected void configure() {}


Done, didn't see an open source Module dependency test to catch this next time

rtannenbaum · 2018-02-28T20:44:12Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+              + " be a local/test project since it doesn't end in -local, -test, or -qa.");
+    } else { // Assume running on GCP
+      // TODO: Check whether we are actually running on GCP once we find out how
+      String credsLocation = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");


rtannenbaum · 2018-02-28T20:45:49Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    } else { // Assume running on GCP
+      // TODO: Check whether we are actually running on GCP once we find out how
+      String credsLocation = System.getenv("GOOGLE_APPLICATION_CREDENTIALS");
+      if (!credsLocation.startsWith("/var/secrets/")) {


rtannenbaum · 2018-02-28T20:54:06Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+    validateUsingGoogle(context);
+    String projectId;
+    try {
+      projectId = System.getenv("GOOGLE_PROJECT_ID");


rtannenbaum · 2018-02-28T20:59:54Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+   * Google cloud?
+   */
+  private void validateUsingGoogle(ExtensionContext context) {
+    if (!context.getConfiguration("cloud", "").equals("GOOGLE")) {


holachuy

Just a few comments

holachuy · 2018-02-28T23:52:25Z

extensions/cloud/portability-cloud-google/build.gradle

@@ -22,4 +22,9 @@ dependencies {
    compile("com.google.cloud:google-cloud-storage:${googleDatastoreVersion}")
    compile("com.google.cloud:google-cloud-datastore:${googleDatastoreVersion}")
    compile("com.google.inject:guice:${guiceVersion}")
+
+    testCompile("org.mockito:mockito-all:${mockitoVersion}")


Maybe we can rearrange in alpha order.

holachuy · 2018-03-01T00:14:29Z

extensions/cloud/portability-cloud-google/build.gradle

    compile("com.google.cloud:google-cloud-storage:${googleDatastoreVersion}")
    compile("com.google.cloud:google-cloud-datastore:${googleDatastoreVersion}")
+    compile("com.google.inject:guice:${guiceVersion}")
+
+    testCompile("org.mockito:mockito-all:${mockitoVersion}")


Maybe we can list these in alpha order?

holachuy · 2018-03-01T00:19:54Z

...ty-cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCloudModule.java

+  // The path where Kubernetes stores Secrets.
+  private static final String KUBERNETES_SECRETS_PATH_ROOT = "/var/secrets/";
+
+  @VisibleForTesting enum Environment {


Since we already have a concept of environment, do we want to call this Realm? Or SubEnvironment? Or something else?

I only saw the definition in Core which I figured we would deprecate. Is there another one yet? I have not rebased your changes yet as to not mess up the code review, but I don't see another Environment in github code search. I will put a TODO for now.

holachuy · 2018-03-01T00:20:35Z

...cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCryptoKeyStore.java

+  private static final String SECRETS_CRYPTO_KEY_FMT_STRING = "projects/%s/locations/global/"
+      + "keyRings/portability_secrets/cryptoKeys/portability_secrets_key";
+
+  private CloudKMS cloudKMS;


these can be final

thanks, I just checked I'm not missing any more, sorry about that

holachuy · 2018-03-01T00:21:02Z

...cloud-google/src/main/java/org/dataportabilityproject/cloud/google/GoogleCryptoKeyStore.java

+
+  @Inject
+  GoogleCryptoKeyManagementSystem(@ProjectId String projectId) throws GoogleCredentialException {
+    HttpTransport transport = new NetHttpTransport();


Make transport injectable or add a TODO to do so

Done, did same with Json factory

rtannenbaum requested a review from holachuy February 23, 2018 21:57

holachuy reviewed Feb 26, 2018

View reviewed changes

rtannenbaum commented Feb 28, 2018

View reviewed changes

holachuy approved these changes Mar 1, 2018

View reviewed changes

rtannenbaum added 5 commits March 1, 2018 08:50

Port Google Cloud classes to the new structure

034cb01

undo accidental change

26083d3

Make names for Local stores consistent with Google stores

bce8856

Code review

7b811f2

Code review

292fd91

rtannenbaum force-pushed the google-stores branch from ee388bf to 292fd91 Compare March 1, 2018 14:53

rtannenbaum merged commit 7a2a6e9 into master Mar 1, 2018

rtannenbaum deleted the google-stores branch March 1, 2018 15:00

Port Google Cloud classes to the new structure #156

Port Google Cloud classes to the new structure #156

Conversation

rtannenbaum commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtannenbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holachuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment