From 04e22d6a329ef462621c7276d7a58dfc6ba5b62c Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 8 Aug 2024 17:20:26 -0400 Subject: [PATCH] Add bandit and pyright checks to test code Use tenacity where it is useful to fix pyright and bandit errors. Change pylint target to lint target in top level Makefile, since the target also includes using bandit and pyright. Signed-off-by: mulhern --- .githooks/pre-commit | 2 +- .github/workflows/main.yml | 10 ++++++-- .github/workflows/support.yml | 8 ++++++- .github/workflows/weekly.yml | 8 ++++++- Makefile | 6 +++-- tests/client-dbus/Makefile | 8 +++++++ .../stratisd_client_dbus/_implementation.py | 18 +++++++++------ tests/client-dbus/tests/udev/_loopback.py | 4 ++++ tests/client-dbus/tests/udev/_utils.py | 2 +- tests/client-dbus/tests/udev/test_predict.py | 2 +- tests/client-dbus/tests/udev/test_revert.py | 23 +++++++++++-------- 11 files changed, 65 insertions(+), 26 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 45f5a07ec6..972cf61439 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -12,7 +12,7 @@ make fmt-ci && make clippy && make yamllint && make tmtlint && - make pylint && + make lint && make check-typos || exit 1 export PYTHONPATH=$PWD/tests/client-dbus/src diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ca03810fa1..7520b17534 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -192,11 +192,17 @@ jobs: - name: Install dependencies run: > dnf install -y + bandit make + pip pylint python3-dbus - - name: Run pylint - run: make -f Makefile pylint + - name: Install pyright + run: pip install --user pyright + - name: Run lint + run: > + PATH=${PATH}:/github/home/.local/bin + make -f Makefile lint python-based-tests: runs-on: ubuntu-22.04 diff --git a/.github/workflows/support.yml b/.github/workflows/support.yml index 0f7adcf087..3534145df6 100644 --- a/.github/workflows/support.yml +++ b/.github/workflows/support.yml @@ -39,6 +39,7 @@ jobs: matrix: include: - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -47,7 +48,9 @@ jobs: python3-pyudev python3-semantic_version python3-tenacity - task: PYTHONPATH=./src make -f Makefile lint + task: > + PATH=${PATH}:/github/home/.local/bin + PYTHONPATH=./src make -f Makefile lint working-directory: ./tests/client-dbus - dependencies: black python3-isort task: make -f Makefile fmt-ci @@ -64,7 +67,10 @@ jobs: dnf install -y make ncurses + pip ${{ matrix.dependencies }} + - name: Install pyright + run: pip install --user pyright - name: Run test run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/.github/workflows/weekly.yml b/.github/workflows/weekly.yml index ebc6d0c872..84ba2d38be 100644 --- a/.github/workflows/weekly.yml +++ b/.github/workflows/weekly.yml @@ -17,6 +17,7 @@ jobs: include: # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -25,7 +26,9 @@ jobs: python3-pyudev python3-semantic_version python3-tenacity - task: PYTHONPATH=./src make -f Makefile lint + task: > + PATH=${PATH}:/github/home/.local/bin + PYTHONPATH=./src make -f Makefile lint working-directory: ./tests/client-dbus - dependencies: black python3-isort task: make -f Makefile fmt-ci @@ -45,10 +48,13 @@ jobs: dnf install -y make ncurses + pip python-unversioned-command ${{ matrix.dependencies }} - name: Display Python version run: python --version + - name: Install pyright + run: pip install --user pyright - name: ${{ matrix.task }} run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/Makefile b/Makefile index 2edbbf197f..425ec7a6cd 100644 --- a/Makefile +++ b/Makefile @@ -430,8 +430,10 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl cargo clippy ${CLIPPY_OPTS} ## Lint Python parts of the source code -pylint: +lint: pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm + bandit ./src/bin/utils/stratis-decode-dm --skip B101 + pyright ./src/bin/utils/stratis-decode-dm .PHONY: audit @@ -477,7 +479,7 @@ pylint: install-udev-binaries install-udev-cfg license - pylint + lint test test-valgrind test-loop diff --git a/tests/client-dbus/Makefile b/tests/client-dbus/Makefile index 8a9cddbf7b..b0528087c0 100644 --- a/tests/client-dbus/Makefile +++ b/tests/client-dbus/Makefile @@ -1,9 +1,17 @@ UNITTEST_OPTS = --verbose +# Ignore bandit B404 errors. Any import of the subprocess module causes this +# error. We know what we are doing when we import that module and do not +# need to be warned. +BANDIT_SKIP = --skip B404,B603,B311 + .PHONY: lint lint: pylint src/stratisd_client_dbus --ignore=_introspect.py pylint tests --disable=R0801 + bandit --recursive ./src ${BANDIT_SKIP} + bandit --recursive ./tests ${BANDIT_SKIP},B101 + pyright .PHONY: startup-tests startup-tests: diff --git a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py index 81dcb4e28b..ef680f0c64 100644 --- a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py +++ b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py @@ -16,7 +16,7 @@ """ # isort: STDLIB -import xml.etree.ElementTree as ET +import xml.etree.ElementTree as ET # nosec B405 # isort: FIRSTPARTY from dbus_client_gen import managed_object_class, mo_query_builder @@ -31,9 +31,9 @@ ) from ._introspect import SPECS -_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) -_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) -_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) +_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) # nosec B314 +_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) # nosec B314 +_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) # nosec B314 pools = mo_query_builder(_POOL_SPEC) filesystems = mo_query_builder(_FILESYSTEM_SPEC) @@ -46,11 +46,15 @@ ObjectManager = make_class( "ObjectManager", - ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), + ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), # nosec B314 TIME_OUT, ) -Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT) -Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT) +Report = make_class( + "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT # nosec B314 +) +Manager = make_class( + "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT # nosec B314 +) Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT) Pool = make_class("Pool", _POOL_SPEC, TIME_OUT) Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT) diff --git a/tests/client-dbus/tests/udev/_loopback.py b/tests/client-dbus/tests/udev/_loopback.py index 94e0a27cca..9b4aafe8d9 100644 --- a/tests/client-dbus/tests/udev/_loopback.py +++ b/tests/client-dbus/tests/udev/_loopback.py @@ -105,6 +105,8 @@ def create_devices(self, number): :return: list of keys for the devices :rtype: list of uuid.UUID """ + assert self.dir is not None + tokens = [] first_creation = self.count == 0 @@ -220,6 +222,8 @@ def destroy_all(self): Detach all the devices and delete the file(s) and directory! :return: None """ + assert self.dir is not None + self.destroy_devices() os.rmdir(self.dir) self.dir = None diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 3182ec41a0..9ea1784835 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -243,7 +243,7 @@ def settle(): :return: None """ time.sleep(2) - subprocess.check_call(["udevadm", "settle"]) + subprocess.check_call(["/usr/bin/udevadm", "settle"]) def wait_for_udev_count(expected_num): diff --git a/tests/client-dbus/tests/udev/test_predict.py b/tests/client-dbus/tests/udev/test_predict.py index c6d52e26db..214c64eeab 100644 --- a/tests/client-dbus/tests/udev/test_predict.py +++ b/tests/client-dbus/tests/udev/test_predict.py @@ -94,7 +94,7 @@ def _call_blockdev_size(dev): :rtype: str """ with subprocess.Popen( - ["blockdev", "--getsize64", dev], + ["/usr/sbin/blockdev", "--getsize64", dev], stdout=subprocess.PIPE, ) as command: outs, _ = command.communicate() diff --git a/tests/client-dbus/tests/udev/test_revert.py b/tests/client-dbus/tests/udev/test_revert.py index db010b5cd2..99bfe00106 100644 --- a/tests/client-dbus/tests/udev/test_revert.py +++ b/tests/client-dbus/tests/udev/test_revert.py @@ -42,6 +42,9 @@ settle, ) +_MOUNT = "/usr/bin/mount" +_UMOUNT = "/usr/bin/umount" + def write_file(mountdir, filename): """ @@ -99,7 +102,7 @@ def test_revert(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -119,7 +122,7 @@ def test_revert(self): # pylint: disable=too-many-locals write_file(mountdir, file2) Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) @@ -131,12 +134,12 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.read_file(mountdir, file2) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) # Now stop the pool, which should tear down the devices (_, return_code, message) = Manager.Methods.StopPool( @@ -167,14 +170,14 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.assertFalse(os.path.exists(os.path.join(mountdir, file2))) self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals """ @@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals Filesystem.Properties.MergeScheduled.Set( get_object(snap_object_path_1), True ) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) @@ -285,7 +288,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) @@ -293,4 +296,4 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir])