Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python: Integrate linting tools #15922

Merged
merged 13 commits into from
Apr 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,4 @@ cmake-build-debug
/linux
bazel.output.txt
*~
.coverage
7 changes: 7 additions & 0 deletions .yapfignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
*generated*
*venv*
*protos*
*~
*_pb2.py
*tests*
*#*
7 changes: 0 additions & 7 deletions bazel/repositories_extra.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,6 @@ load("@proxy_wasm_cpp_host//bazel/cargo:crates.bzl", "proxy_wasm_cpp_host_fetch_
def _python_deps():
py_repositories()

# REMOVE!!!
pip_install(
name = "sometools_pip3",
requirements = "@envoy//tools/sometools:requirements.txt",
extra_pip_args = ["--require-hashes"],
)

pip_install(
name = "config_validation_pip3",
requirements = "@envoy//tools/config_validation:requirements.txt",
Expand Down
8 changes: 5 additions & 3 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ set -e

build_setup_args=""
if [[ "$1" == "format_pre" || "$1" == "fix_format" || "$1" == "check_format" || "$1" == "check_repositories" || \
"$1" == "check_spelling" || "$1" == "fix_spelling" || "$1" == "bazel.clang_tidy" || \
"$1" == "check_spelling_pedantic" || "$1" == "fix_spelling_pedantic" ]]; then
build_setup_args="-nofetch"
"$1" == "check_spelling" || "$1" == "fix_spelling" || "$1" == "bazel.clang_tidy" || "$1" == "tooling" || \
"$1" == "check_spelling_pedantic" || "$1" == "fix_spelling_pedantic" ]]; then
build_setup_args="-nofetch"
fi

# TODO(phlax): Clarify and/or integrate SRCDIR and ENVOY_SRCDIR
Expand Down Expand Up @@ -470,8 +470,10 @@ elif [[ "$CI_TARGET" == "tooling" ]]; then
echo "Run pytest tooling tests..."
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/testing:pytest_python_pytest -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/testing:pytest_python_coverage -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/base:pytest_checker -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/base:pytest_runner -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/base:pytest_utils -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/code_format:pytest_python_check -- --cov-collect /tmp/.coverage-envoy
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/testing:python_coverage -- --fail-under=95 /tmp/.coverage-envoy /source/generated/tooling
exit 0
elif [[ "$CI_TARGET" == "verify_examples" ]]; then
Expand Down
12 changes: 1 addition & 11 deletions ci/format_pre.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,8 @@ CURRENT=shellcheck
CURRENT=configs
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //configs:example_configs_validation

# TODO(phlax): update to use bazel and python_flake8/python_check
# this will simplify this code to a single line
CURRENT=python
"${ENVOY_SRCDIR}"/tools/code_format/format_python_tools.sh check || {
"${ENVOY_SRCDIR}"/tools/code_format/format_python_tools.sh fix
git diff HEAD | tee "${DIFF_OUTPUT}"
raise () {
# this throws an error without exiting
return 1
}
raise
}
bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/code_format:python_check -- --diff-file="$DIFF_OUTPUT" --fix "$(pwd)"

if [[ "${#FAILED[@]}" -ne "0" ]]; then
echo "TESTS FAILED:" >&2
Expand Down
101 changes: 92 additions & 9 deletions tools/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ We will assume that `sometools` does not yet exist and will also need a `require
and `bazel` rule to configure the dependencies.

In most cases of adding a tool, it is likely you will not need to create a new set of dependencies, and
you can skip to the ("Add Python requirements")[#add-python-requirements] section.
you can skip to the ["Add Python requirements"](#add-python-requirements) section.

We will also assume that you have `python3` and `pip` installed and working in your local environment.

Expand Down Expand Up @@ -149,8 +149,6 @@ add a test runner to ensure the file is tested.
```starlark
envoy_py_binary(
name = "tools.sometools.mytool",
srcs = ["mytool.py"],
visibility = ["//visibility:public"],
deps = [
requirement("requests"),
requirement("pyyaml"),
Expand Down Expand Up @@ -229,7 +227,7 @@ def test_mytool_main():
This example use the mock library to patch all of the method calls, and
then tests that they have been called with the expected values.

You can run the test as follows:
You can run the test using the (automatically generated) `//tools/sometools:pytest_mytool` target as follows:

```console
$ bazel run //tools/sometools:pytest_mytool
Expand Down Expand Up @@ -314,15 +312,13 @@ This will drop you into the Python debugger (`pdb`) at the breakpoint.

A base class for writing tools that need to parse command line arguments has been provided.

To make use of it in this example we will need to add the runner as a dependency to the `tools.base.mytool` target.
To make use of it in this example we will need to add the runner as a dependency to the `tools.sometools.mytool` target.

Edit `tools/sometools/BUILD` and change the `tools.base.mytool` target to the following:
Edit `tools/sometools/BUILD` and change the `tools.sometools.mytool` target to the following:

```starlark
envoy_py_binary(
name = "tools.base.mytool",
srcs = ["mytool.py"],
visibility = ["//visibility:public"],
name = "tools.sometools.mytool",
deps = [
"//tools/base:runner",
requirement("requests"),
Expand Down Expand Up @@ -389,3 +385,90 @@ or directly with `python`:
$ ./tools/sometools/mytool.py -h
...
```

### Using the `tools.base.checker.Checker` class

A base class for writing checkers (for example, linting tools) has also been provided.

Any classes subclassing `tools.base.checker.Checker` should provide a tuple of `__class__.checks`.

For each named check in `checks` the `Checker` will expect a method of the same name with the prefix `check_`.

For example, setting `checks` to the tuple `("check1", "check2")` the `Checker` will run the methods `check_check1` and `check_check2` in order.

Let's look at an example.

First, we need to add the bazel target.

Edit `tools/sometools/BUILD` and add a `tools.sometools.mychecker` target with a dependency on the base `Checker`.

```starlark
envoy_py_binary(
name = "tools.sometools.mychecker",
deps = [
"//tools/base:checker",
],
)
```

Next add the `MyChecker` class to `tools/sometools/mychecker.py` as follows:

```python
#!/usr/bin/env python3

import sys

from tools.base.checker import Checker


class MyChecker(Checker):
checks = ("check1", "check2")

def check_check1(self) -> None:
# checking code for check1
try:
do_something()
except NotSuchABadError:
self.warn("check1", ["Doing something didn't work out quite as expected 8/"])
except ATerribleError:
self.error("check1", ["Oh noes, something went badly wrong! 8("])
else:
self.succeed("check1", ["All good 8)"])

def check_check2(self) -> None:
# checking code for check2
try:
do_something_else()
except NotSuchABadError:
self.warn("check2", ["Doing something else didn't work out quite as expected 8/"])
except ATerribleError:
self.error("check2", ["Oh noes, something else went badly wrong! 8("])
else:
self.succeed("check2", ["All good 8)"])


def main(*args) -> int:
return MyChecker(*args).run()


if __name__ == "__main__":
sys.exit(main(*sys.argv[1:]))
```

Just like with the `Runner` class described [above](#using-the-toolsbaserunnerrunner-class) you can
use both with and without `bazel`. To use without, you will need make it executable, and the end
user will need to have any dependencies locally installed.

Notice in the check methods the results of the check are logged to one of `self.error`, `self.warn`,
or `self.succeed`. Each takes a `list` of outcomes. The results will be summarized to the user at the
end of all checks.

Just like with `Runner` a help menu is automatically created, and you can add custom arguments if
required.

Also like `Runner`, any added `Checker` classes are expected to have unit tests, and a `pytest_mychecker` bazel target
is automatically added. With the above example, the test file should be located at `tools/sometools/tests/test_mychecker.py`.

One key difference with the `Checker` tools and its derivatives is that it expects a `path` either specified
with `--path` or as an argument. This is used as a context (for example the Envoy src directory) for
running the checks.
7 changes: 7 additions & 0 deletions tools/base/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,10 @@ envoy_package()
envoy_py_library("tools.base.runner")

envoy_py_library("tools.base.utils")

envoy_py_library(
"tools.base.checker",
deps = [
":runner",
],
)
Loading