[GitHub] [beam] kennknowles commented on a diff in pull request #27579: Add Integration/Load test Utilities.

["kennknowles (via GitHub)" <gi...@apache.org>]

kennknowles commented on code in PR #27579:
URL: https://github.com/apache/beam/pull/27579#discussion_r1269770647


##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -813,6 +814,7 @@ class BeamModulePlugin implements Plugin<Project> {
         testcontainers_mysql                        : "org.testcontainers:mysql:$testcontainers_version",
         testcontainers_postgresql                   : "org.testcontainers:postgresql:$testcontainers_version",
         testcontainers_rabbitmq                     : "org.testcontainers:rabbitmq:$testcontainers_version",
+        truth                                       : "com.google.truth:truth:1.0.1",

Review Comment:
   I know that Google prefers truth internally. Beam already pulls in hamcrest and JUnit for everything. It may create an annoying situation to have all these very similar frameworks in the project. Is there a special reason to use truth   now?



##########
it/google-cloud-platform/src/main/java/org/apache/beam/it/gcp/GCPBaseIT.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.it.gcp;
+
+import com.google.api.gax.core.CredentialsProvider;
+import com.google.api.gax.core.FixedCredentialsProvider;
+import com.google.auth.Credentials;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.apache.beam.it.common.TestProperties;
+import org.junit.Before;
+
+@SuppressWarnings("nullness")

Review Comment:
   Ah, this is interesting. Because it is in `src/main` it is not automatically skipped. [We do have checker disabled in all tests](https://github.com/apache/beam/blob/51512cadf2d65c1e0dc3f19e3287759a1dd78b2f/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L1098), because it does not work well with JUnit APIs.
   
   I don't have enough context on the design of the modules to know if we should change anything here, so I am just sharing this as an FYI that we may want to work on.



##########
it/google-cloud-platform/build.gradle:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+plugins { id 'org.apache.beam.module' }
+applyJavaNature(
+        automaticModuleName: 'org.apache.beam.it.gcp',
+)
+
+description = "Apache Beam :: IT :: Google Cloud Platform"
+ext.summary = "Integration test utilities for Google Cloud Platform."
+
+dependencies {
+    implementation enforcedPlatform(library.java.google_cloud_platform_libraries_bom)
+    implementation project(path: ":sdks:java:core", configuration: "shadow")
+    implementation project(path: ":runners:google-cloud-dataflow-java")
+    implementation project(path: ":it:conditions", configuration: "shadow")
+    implementation project(path: ":it:truthmatchers", configuration: "shadow")
+    implementation library.java.slf4j_api
+    implementation library.java.vendored_guava_26_0_jre
+    implementation library.java.jackson_core
+    implementation library.java.jackson_databind
+    implementation 'org.apache.hadoop:hadoop-common:3.3.5'
+    implementation 'org.apache.avro:avro:1.11.1'
+    implementation 'org.apache.parquet:parquet-avro:1.12.0'
+    implementation 'org.apache.parquet:parquet-common:1.12.0'
+    implementation 'org.apache.parquet:parquet-hadoop:1.12.0'
+    implementation library.java.gax
+    implementation library.java.google_api_common
+    implementation library.java.protobuf_java_util
+    implementation library.java.protobuf_java
+    implementation 'org.threeten:threetenbp:1.6.8'

Review Comment:
   Even though joda time is deprecated, it is what Beam uses. We should use it here too, and change as part of a global process. Is there a reason that we cannot?
   
   And if we keep threeten then it should go in the global list of dependencies. But unless there is a special compatibility reason with other libraries we should use joda.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org