[GitHub] [druid] paul-rogers commented on a diff in pull request #13864: Query tests without TLS

["paul-rogers (via GitHub)" <gi...@apache.org>]

paul-rogers commented on code in PR #13864:
URL: https://github.com/apache/druid/pull/13864#discussion_r1122121991


##########
.github/workflows/revised-its.yml:
##########
@@ -36,3 +36,20 @@ jobs:
       script: ./it.sh github ${{ matrix.it }}
       it: ${{ matrix.it }}
       mysql_driver: com.mysql.jdbc.Driver
+
+  query-it:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ 8 ]
+        it: [ Query, QueryRetry, QueryError ]
+        indexer: [ middleManager ]
+    uses: ./.github/workflows/reusable-revised-its.yml
+    with:
+      build_jdk: ${{ matrix.jdk }}
+      runtime_jdk: ${{ matrix.jdk }}
+      use_indexer: ${{ matrix.indexer }}
+      script: ./it.sh github ${{ matrix.it }}
+      it: ${{ matrix.it }}
+      mysql_driver: com.mysql.jdbc.Driver
+      OVERRIDE_ENV: cluster/Common/prepopulated-data.env

Review Comment:
   This is correct, and thanks much for figuring out the GHA scripts here. We should always think about the developer experience as well. We want to be able to:
   
   * Build Druid: `it.sh build`
   * Build the image: `it.sh image`
   * Start a test cluster: `it.sh up <category>`
   * Debug test in an IDE.
   
   As a result, the MySQL driver should default to MySQL. If it does, there is no need to set it here.
   
   If a test requires pre-populated data, that should appear in the cluster setup scripts, not a GHA file. The only reason an env override should be in GHA is if we want to do something _different_ than a developer would do "out of the box." Something like data is critical: all tests must use the same data or they cannot pass.
   
   It may be the case that the "old" ITs used the approach here, so I'm suggesting we actually change the approach to improve the developer experience.
   
   How can we change this to push these two items into the test setup?



##########
integration-tests-ex/cases/.gitignore:
##########
@@ -1 +1,7 @@
 /bin/
+client_tls/
+override.env
+tls/root.key
+tls/root.pem
+tls/untrusted_root.key
+tls/untrusted_root.pem

Review Comment:
   Let's put these files in `target` not the source directory. I realize the old ITs left them in the source directory. Doing so is a hassle for developers: it is a bug, not a feature.
   
   Also, are these files part of this PR?



##########
integration-tests-ex/cases/cluster/Query/docker-compose.yaml:
##########
@@ -0,0 +1,98 @@
+# 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.
+
+networks:

Review Comment:
   The code to generate `docker-compose.yaml` files is now in place. Hand-written files still work. So, let's get these hand-written files in, then I'll convert them to the generated format.



##########
integration-tests-ex/cases/cluster.sh:
##########
@@ -111,17 +111,17 @@ function category {
 # Dump lots of information to debug Docker failures when run inside
 # of a build environment where we can't inspect Docker directly.
 function show_status {
-  echo "===================================="
-  ls -l target/shared
-  echo "docker ps -a"
-  docker ps -a
-  # Was: --filter status=exited
-  for id in $(docker ps -a --format "{{.ID}}"); do
+	echo "===================================="
+	ls -l $SHARED_DIR
+	echo "docker ps -a"
+	docker ps -a
+	# Was: --filter status=exited
+	for id in $(docker ps -a --format "{{.ID}}"); do
     echo "===================================="
-    echo "Logs for Container ID $id"
-    docker logs $id | tail -n 20
-  done
-  echo "===================================="
+    echo "Logs for Container ID $id, Name - $(docker inspect --format='{{.Name}}' $id)"
+		docker logs $id | tail -n 20
+	done
+	echo "===================================="

Review Comment:
   Thanks for cleaning up spacing. Druid prefers an indentation of 2 in Java, I've been using that in scripts as well. Regardless of the level chose, the contents of the `for` loop should be indented an extra level.



##########
integration-tests-ex/cases/cluster/Common/prepopulated-data.env:
##########
@@ -0,0 +1,24 @@
+#
+# 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.
+#
+
+AWS_REGION=us-east-1
+
+# Setting s3 credentials and region to use pre-populated data for testing.
+druid_s3_accessKey=AKIAT2GGLKKJQCMG64V4
+druid_s3_secretKey=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

Review Comment:
   Per the note above, can we set this up so that it is the default, which a run can override if it has some other location it wishes to use? That helps with starting a cluster with `it.sh`.



-- 
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: commits-unsubscribe@druid.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org