xds-k8s: Add code quality helpers (#26553)

* xds-k8s: add yapf helper
* add isort helper
* Remove workaround for Segmentation fault imports
* Run isort
* Update README.md

tools/run_tests/xds_k8s_test_driver/README.md

@@ -19,7 +19,7 @@ changes to this codebase at the moment.
 - [ ] Restructure `framework.test_app` and `framework.xds_k8s*` into a module
       containing xDS-interop-specific logic
 - [ ] Address inline TODOs in code
-- [ ] Improve README.md documentation, explain helpers in bin/ folder
+- [x] Improve README.md documentation, explain helpers in bin/ folder
 
 ## Installation
 
@@ -226,6 +226,10 @@ from your dev environment. You need:
    will automatically start and stop port forwarding using
    `kubectl` subprocesses. (experimental)
 
+### Making changes to the driver
+1. Install additional dev packages: `pip install -r requirements-dev.txt`
+2. Use `./bin/yapf.sh` and `./bin/isort.sh` helpers to auto-format code.
+
 ### Setup test configuration
 
 There are many arguments to be passed into the test run. You can save the

tools/run_tests/xds_k8s_test_driver/bin/cleanup.sh

@@ -41,7 +41,8 @@ if [[ "$1" == "-h" || "$1" == "--help" ]]; then
   display_usage
 fi
 
-readonly SCRIPT_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+SCRIPT_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+readonly SCRIPT_DIR
 readonly XDS_K8S_DRIVER_DIR="${SCRIPT_DIR}/.."
 
 cd "${XDS_K8S_DRIVER_DIR}"

tools/run_tests/xds_k8s_test_driver/bin/ensure_venv.sh

@@ -0,0 +1,29 @@
+#!/usr/bin/env bash
+# Copyright 2021 gRPC authors.
+#
+# Licensed 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.
+
+# Expected $XDS_K8S_DRIVER_DIR to be set by the file sourcing this.
+readonly XDS_K8S_DRIVER_VENV_DIR="${XDS_K8S_DRIVER_VENV_DIR:-$XDS_K8S_DRIVER_DIR/venv}"
+
+if [[ -z "${VIRTUAL_ENV}" ]]; then
+  if [[ -d "${XDS_K8S_DRIVER_VENV_DIR}" ]]; then
+    # Intentional: No need to check python venv activate script.
+    # shellcheck source=/dev/null
+    source "${XDS_K8S_DRIVER_VENV_DIR}/bin/activate"
+  else
+    echo "Missing python virtual environment directory: ${XDS_K8S_DRIVER_VENV_DIR}" >&2
+    echo "Follow README.md installation steps first." >&2
+    exit 1
+  fi
+fi

tools/run_tests/xds_k8s_test_driver/bin/isort.sh

@@ -0,0 +1,61 @@
+#!/usr/bin/env bash
+# Copyright 2021 gRPC authors.
+#
+# Licensed 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.
+
+set -eo pipefail
+
+display_usage() {
+  cat <<EOF >/dev/stderr
+A helper to run isort import sorter.
+
+USAGE: $0 [--diff]
+   --diff: Do not apply changes, only show the diff
+
+ENVIRONMENT:
+   XDS_K8S_DRIVER_VENV_DIR: the path to python virtual environment directory
+                            Default: $XDS_K8S_DRIVER_DIR/venv
+EXAMPLES:
+$0
+$0 --diff
+EOF
+  exit 1
+}
+
+if [[ "$1" == "-h" || "$1" == "--help" ]]; then
+  display_usage
+fi
+
+SCRIPT_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+readonly SCRIPT_DIR
+readonly XDS_K8S_DRIVER_DIR="${SCRIPT_DIR}/.."
+
+cd "${XDS_K8S_DRIVER_DIR}"
+
+# Relative paths not yet supported by shellcheck.
+# shellcheck source=/dev/null
+source "${XDS_K8S_DRIVER_DIR}/bin/ensure_venv.sh"
+
+if [[ "$1" == "--diff" ]]; then
+  readonly MODE="--diff"
+else
+  readonly MODE="--overwrite-in-place"
+fi
+
+# typing is the only module allowed to put imports on the same line:
+# https://google.github.io/styleguide/pyguide.html#313-imports-formatting
+exec python -m isort "${MODE}" \
+  --force-sort-within-sections \
+  --force-single-line-imports --single-line-exclusions=typing \
+  framework bin tests
+

tools/run_tests/xds_k8s_test_driver/bin/yapf.sh

@@ -0,0 +1,58 @@
+#!/usr/bin/env bash
+# Copyright 2021 gRPC authors.
+#
+# Licensed 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.
+
+set -eo pipefail
+
+display_usage() {
+  cat <<EOF >/dev/stderr
+A helper to run yapf formatter.
+
+USAGE: $0 [--diff]
+   --diff: Do not apply changes, only show the diff
+
+ENVIRONMENT:
+   XDS_K8S_DRIVER_VENV_DIR: the path to python virtual environment directory
+                            Default: $XDS_K8S_DRIVER_DIR/venv
+EXAMPLES:
+$0
+$0 --diff
+EOF
+  exit 1
+}
+
+if [[ "$1" == "-h" || "$1" == "--help" ]]; then
+  display_usage
+fi
+
+SCRIPT_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+readonly SCRIPT_DIR
+readonly XDS_K8S_DRIVER_DIR="${SCRIPT_DIR}/.."
+
+cd "${XDS_K8S_DRIVER_DIR}"
+
+# Relative paths not yet supported by shellcheck.
+# shellcheck source=/dev/null
+source "${XDS_K8S_DRIVER_DIR}/bin/ensure_venv.sh"
+
+if [[ "$1" == "--diff" ]]; then
+  readonly MODE="--diff"
+else
+  readonly MODE="--in-place"
+  readonly VERBOSE="--verbose" # print out file names while processing
+fi
+
+exec python -m yapf "${MODE}" ${VERBOSE:-} \
+  --parallel --recursive --style=../../../setup.cfg \
+  framework bin tests

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/api.py

@@ -15,20 +15,16 @@ import abc
 import contextlib
 import functools
 import logging
-from typing import Optional, List, Dict, Any
+from typing import Any, Dict, List, Optional
 
-# Workaround: `grpc` must be imported before `google.protobuf.json_format`,
-# to prevent "Segmentation fault". Ref https://github.com/grpc/grpc/issues/24897
-# TODO(sergiitk): Remove after #24897 is solved
-import grpc  # noqa pylint: disable=unused-import
 from absl import flags
 from google.cloud import secretmanager_v1
 from google.longrunning import operations_pb2
 from google.protobuf import json_format
 from google.rpc import code_pb2
 from googleapiclient import discovery
-import googleapiclient.http
 import googleapiclient.errors
+import googleapiclient.http
 import tenacity
 import yaml
 

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/compute.py

@@ -14,7 +14,7 @@
 import dataclasses
 import enum
 import logging
-from typing import Any, Dict, Optional, List
+from typing import Any, Dict, List, Optional
 
 from googleapiclient import discovery
 import googleapiclient.errors

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_security.py

@@ -11,9 +11,9 @@
 # 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.
+import dataclasses
 import logging
 
-import dataclasses
 from google.rpc import code_pb2
 import tenacity
 

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/gcp/network_services.py

@@ -11,10 +11,10 @@
 # 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.
+import dataclasses
 import logging
 from typing import Optional
 
-import dataclasses
 from google.rpc import code_pb2
 import tenacity
 

tools/run_tests/xds_k8s_test_driver/framework/infrastructure/k8s.py

@@ -16,13 +16,13 @@ import json
 import logging
 import subprocess
 import time
-from typing import Optional, List, Tuple
+from typing import List, Optional, Tuple
 
-# TODO(sergiitk): replace with tenacity
-import retrying
-import kubernetes.config
 from kubernetes import client
 from kubernetes import utils
+import kubernetes.config
+# TODO(sergiitk): replace with tenacity
+import retrying
 
 logger = logging.getLogger(__name__)
 # Type aliases

tools/run_tests/xds_k8s_test_driver/framework/rpc/grpc.py

@@ -15,11 +15,9 @@ import logging
 import re
 from typing import ClassVar, Dict, Optional
 
-# Workaround: `grpc` must be imported before `google.protobuf.json_format`,
-# to prevent "Segmentation fault". Ref https://github.com/grpc/grpc/issues/24897
-import grpc
 from google.protobuf import json_format
 import google.protobuf.message
+import grpc
 
 logger = logging.getLogger(__name__)
 

tools/run_tests/xds_k8s_test_driver/framework/rpc/grpc_csds.py

@@ -16,20 +16,20 @@ This contains helpers for gRPC services defined in
 https://github.com/envoyproxy/envoy/blob/main/api/envoy/service/status/v3/csds.proto
 """
 
-import queue
 import logging
-from typing import Optional, Callable
-
-import grpc
-from envoy.service.status.v3 import csds_pb2
-from envoy.service.status.v3 import csds_pb2_grpc
+import queue
+from typing import Callable, Optional
 
-# Envoy protos provided by PyPI package xds-protos
-# Needs to import the generated Python file to load descriptors
-from envoy.extensions.filters.network.http_connection_manager.v3 import http_connection_manager_pb2
 from envoy.extensions.filters.common.fault.v3 import fault_pb2
 from envoy.extensions.filters.http.fault.v3 import fault_pb2
 from envoy.extensions.filters.http.router.v3 import router_pb2
+# Envoy protos provided by PyPI package xds-protos
+# Needs to import the generated Python file to load descriptors
+from envoy.extensions.filters.network.http_connection_manager.v3 import \
+    http_connection_manager_pb2
+from envoy.service.status.v3 import csds_pb2
+from envoy.service.status.v3 import csds_pb2_grpc
+import grpc
 
 import framework.rpc
 

tools/run_tests/xds_k8s_test_driver/framework/rpc/grpc_testing.py

@@ -21,8 +21,8 @@ from typing import Optional
 import grpc
 
 import framework.rpc
-from src.proto.grpc.testing import test_pb2_grpc
 from src.proto.grpc.testing import messages_pb2
+from src.proto.grpc.testing import test_pb2_grpc
 
 # Type aliases
 _LoadBalancerStatsRequest = messages_pb2.LoadBalancerStatsRequest

tools/run_tests/xds_k8s_test_driver/framework/xds_k8s_testcase.py

@@ -17,10 +17,10 @@ import hashlib
 import logging
 import time
 from typing import Optional, Tuple
-from google.protobuf import json_format
 
 from absl import flags
 from absl.testing import absltest
+from google.protobuf import json_format
 
 from framework import xds_flags
 from framework import xds_k8s_flags

tools/run_tests/xds_k8s_test_driver/requirements-dev.txt

@@ -0,0 +1,4 @@
+-r requirements.txt
+yapf==0.30.0  # Mirrors yapf version set in https://github.com/grpc/grpc/blob/master/tools/distrib/yapf_code.sh
+isort~=5.9
+# TODO(https://github.com/grpc/grpc/pull/25872): mypy

tools/run_tests/xds_k8s_test_driver/run.sh

@@ -15,8 +15,8 @@
 
 set -eo pipefail
 
-readonly XDS_K8S_DRIVER_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
-readonly XDS_K8S_DRIVER_VENV_DIR="${XDS_K8S_DRIVER_VENV_DIR:-$XDS_K8S_DRIVER_DIR/venv}"
+XDS_K8S_DRIVER_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+readonly XDS_K8S_DRIVER_DIR
 readonly XDS_K8S_CONFIG="${XDS_K8S_CONFIG:-$XDS_K8S_DRIVER_DIR/config/local-dev.cfg}"
 
 display_usage() {
@@ -54,17 +54,9 @@ if [[ "$#" -eq 0 || "$1" = "-h" || "$1" = "--help" ]]; then
   display_usage
 fi
 
-if [[ -z "${VIRTUAL_ENV}" ]]; then
-  if [[ -d "${XDS_K8S_DRIVER_VENV_DIR}" ]]; then
-    # Intentional: No need to check python venv activate script.
-    # shellcheck source=/dev/null
-    source "${XDS_K8S_DRIVER_VENV_DIR}/bin/activate"
-  else
-    echo "Missing python virtual environment directory: ${XDS_K8S_DRIVER_VENV_DIR}" >&2
-    echo "Follow README.md installation steps first." >&2
-    exit 1
-  fi
-fi
+# Relative paths not yet supported by shellcheck.
+# shellcheck source=/dev/null
+source "${XDS_K8S_DRIVER_DIR}/bin/ensure_venv.sh"
 
 cd "${XDS_K8S_DRIVER_DIR}"
 export PYTHONPATH="${XDS_K8S_DRIVER_DIR}"