Configure ruff #1028

micahflee · 2024-05-08T18:43:18Z

Status

Ready for review

Description of Changes

This replaces flake8, bandit, black, and isort with ruff!

Many of the things that ruff flagged were assert statements in validate_config.py, and I already fixed those in #1025. So rather than fixing them again here, I merged that branch into this branch. So please review and merge #1025 first.

I started copying the ruff config from freedomofpress/securedrop-client#1885, but then I paired it down to only include what's needed for this repo.

I added S108 (probably insecure usage of /tmp) to the ignore list, as it was being triggered just on this code in sdw_updater/Updater.py:

# We use a hardcoded temporary directory path in dom0. As dom0 is not
# a multi-user environment, we can safely assume that only the Updater is
# managing that filepath. Later on, we should consider porting the check-migration
# logic to leverage the Qubes Python API.
MIGRATION_DIR = "/tmp/sdw-migrations"  # nosec

I also added a per-file-ignore for SIM115 (use context handler for opening files) for sdw_util/Util.py specifically for the obtain_lock and can_obtain_lock functions. This check wants you to open files using the with statement so they automatically close, but in this case these functions open files and then return the file handle. So I think making an exception is fine.

Though I did notice a potential small bug. In can_obtain_lock, the lock file is opened temporarily, just to see if it can be opened, but I don't think it's ever closed since it returns a bool instead of the file handle -- though maybe this is fine and I just don't understand how fcntl.lockf works:

def can_obtain_lock(basename):
    """
    We temporarily obtain a shared, nonblocking lock to a lockfile to determine
    whether the associated process is currently running. Returns True if it is
    safe to continue execution (no lock conflict), False if not.

    `basename` is the basename of a lockfile situated in the LOCK_DIRECTORY.
    """
    lock_file = os.path.join(LOCK_DIRECTORY, basename)
    try:
        lh = open(lock_file)
    except FileNotFoundError:
        # Process may not have run during this session, safe to continue
        return True

    try:
        # Obtain a nonblocking, shared lock
        fcntl.lockf(lh, fcntl.LOCK_SH | fcntl.LOCK_NB)
    except OSError:
        sdlog.error(LOCK_ERROR.format(lock_file))
        return False

    return True

files/sdw-admin.py

rocodes · 2024-05-10T14:50:52Z

sdw_updater/Updater.py

@@ -361,12 +352,12 @@ def overall_update_status(results):

    if updates_failed:
        return UpdateStatus.UPDATES_FAILED
-    elif reboot_required:
+    if reboot_required:


(this one's always gonna be weird for me because I feel like the elif is more readable and more like defensive coding even though it's not technically necessary. but okay, ruff.)

Actually the securedrop-client project ignores these specific ruff rules specifically for readability: https://github.com/freedomofpress/securedrop-client/blob/main/pyproject.toml#L67C1-L68

I ended up not ignoring them here just because it only affected a few places. But we could certainly ignore them here too if you want.

Thanks for pointing that out, it explains why this stuck out at me so much.

Sorry to nitpick, but for the sake of consistency, let's add back the rules that are specifically about readability, so that we have the same standards on all projects.

So that would be, to my eye

# superflous-else- rules, find they hurt readability "RET505", "RET506", "RET507", "RET508", # Find contextlib.suppress() is harder to read "SIM105", # Find ternary statements harder to read "SIM108", # Using any()/all() can be harder to read "SIM110",

Can we re-run the automated fixes now that those rules are suppressed? i.e. back out these changes here and elsewhere

rocodes · 2024-05-10T14:57:29Z

Thanks for preparing this, this looks great. I'm going to look into the lock file issue now. We use the lockfile only in the Updater/+Notifier, eg to make sure that we're not bugging the user to run updates while they're already running updates.

(Edit: I remember something vaguely upsetting, like closing the file will remove all locks. But I'm trying to refresh my memory.)

rocodes · 2024-05-10T15:20:14Z

tldr: good bugfinding eye, but as far as I can tell, we can leave the lock file logic as is. The lock is attached to the life of the process (in our case the updater or notifier job), and closing the file would release any locks that were being held by another process. 😒 The same process that obtains the shared lock can obtain the exclusive lock later.

more:
This from an article called "everything you never wanted to know about locks" describes the difference between fcntl and flock for locking:

The second strange behaviour of fcntl() locks is this: the lock doesn't belong to a file descriptor, it belongs to a (pid,inode) pair. Worse, if you close any fd referring to that inode, it releases all your locks on that inode. For example, let's say you open /etc/passwd and lock some bytes, and then call getpwent() in libc, which usually opens /etc/passwd, reads some stuff, and closes it again. That process of opening /etc/passwd and closing it - which has nothing to do with your existing fd open on /etc/passwd - will cause you to lose your locks!

the python docs weren't as illuminating but this other article on lwn was also helpful. So....I think this is okay, specifically because of the short-lived nature of the updater and notifier, as opposed to obtaining a temporary lock that should not last the length of eg a long-running application. But we could comment or document this issue better, and can_obtain_lock does obtain lock, which isn't the best naming convention.

rocodes

Sorry to punt this back to you for just a small fix while our eyes are here - otherwise LGTM

The merge-base changed after approval.

legoktm

I rebased this now that the assert PR has landed and left some comments. One other thing is that the # nosec comments needed for bandit can now be removed.

legoktm · 2024-05-14T22:12:46Z

Makefile

-	poetry run isort .
+.PHONY: check-ruff
+check-ruff: ## Check Python source code formatting with ruff
+	poetry run ruff check


I think we also need to check ruff format - https://github.com/freedomofpress/securedrop-client/blob/2e2fdca7d280044e54578c5d4a6643a3207c77e5/Makefile#L34

legoktm · 2024-05-14T22:14:10Z

files/destroy-vm.py

@@ -38,10 +39,11 @@ def destroy_vm(vm):
    Destroys a single VM instance. Requires arg to be
    QubesVM object.
    """
-    assert SDW_DEFAULT_TAG in vm.tags
+    if SDW_DEFAULT_TAG not in vm.tags:
+        raise Exception


Can we have an error message here?

legoktm · 2024-05-14T22:29:55Z

Makefile

-flake8: ## Validate PEP8 compliance for Python source files
-	poetry run flake8
+.PHONY: ruff
+ruff: ## Fix Python source code formatting with ruff


Can we either call this fix or alias that to this? In the spirit of freedomofpress/securedrop-tooling#11.

legoktm · 2024-05-14T22:30:51Z

launcher/tests/test_updater.py

-    assert contents == current_time
+    with open(flag_file_dom0) as f:
+        contents = f.read()
+        assert contents == current_time


the assert can happen outside the with statement

legoktm · 2024-05-14T22:32:54Z

sdw_updater/Updater.py

    # Figure out how much each completed VM should bump the progress bar.
-    assert progress_end > progress_start
+    if progress_end <= progress_start:
+        raise Exception


same, can we have an error message here?

legoktm · 2024-05-14T22:34:13Z

sdw_updater/Updater.py

@@ -361,12 +352,12 @@ def overall_update_status(results):

    if updates_failed:
        return UpdateStatus.UPDATES_FAILED
-    elif reboot_required:
+    if reboot_required:


Can we re-run the automated fixes now that those rules are suppressed? i.e. back out these changes here and elsewhere

micahflee added the good first issue label May 8, 2024

rocodes self-requested a review May 10, 2024 14:07

rocodes reviewed May 10, 2024

View reviewed changes

files/sdw-admin.py Outdated Show resolved Hide resolved

rocodes reviewed May 10, 2024

View reviewed changes

rocodes requested changes May 10, 2024

View reviewed changes

micahflee requested a review from rocodes May 10, 2024 16:27

rocodes previously approved these changes May 13, 2024

View reviewed changes

rocodes enabled auto-merge May 13, 2024 17:46

micahflee added 9 commits May 14, 2024 18:19

Add ruff and remove flake8 and black

b33565d

Update ruff config

a8bcc42

Automatic ruff fixes

0af4c23

Fix ruff suggestions

588abac

Remove flake8 and black from Makefile, and replace with ruff

a8fbc36

Add an exception for sdw_util/Util.py

4d2430c

Remove isort and bandit, since ruff takes care of these now

7225541

In install_pvh_support(), use subprocess.check_call instead of .run

3c0a71f

Add readability ruff rules

04e1189

legoktm force-pushed the 1022-ruff branch from f7ab8fc to 04e1189 Compare May 14, 2024 22:28

legoktm requested changes May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure ruff #1028

Configure ruff #1028

micahflee commented May 8, 2024

rocodes May 10, 2024

micahflee May 10, 2024

rocodes May 10, 2024 •

edited

legoktm May 14, 2024 •

edited

rocodes commented May 10, 2024 •

edited

rocodes commented May 10, 2024

rocodes left a comment

legoktm left a comment

legoktm May 14, 2024

legoktm May 14, 2024

legoktm May 14, 2024

legoktm May 14, 2024

legoktm May 14, 2024

legoktm May 14, 2024 •

edited

Configure ruff #1028

Are you sure you want to change the base?

Configure ruff #1028

Conversation

micahflee commented May 8, 2024

Status

Description of Changes

rocodes May 10, 2024

Choose a reason for hiding this comment

micahflee May 10, 2024

Choose a reason for hiding this comment

rocodes May 10, 2024 • edited

Choose a reason for hiding this comment

legoktm May 14, 2024 • edited

Choose a reason for hiding this comment

rocodes commented May 10, 2024 • edited

rocodes commented May 10, 2024

rocodes left a comment

Choose a reason for hiding this comment

legoktm left a comment

Choose a reason for hiding this comment

legoktm May 14, 2024

Choose a reason for hiding this comment

legoktm May 14, 2024

Choose a reason for hiding this comment

legoktm May 14, 2024

Choose a reason for hiding this comment

legoktm May 14, 2024

Choose a reason for hiding this comment

legoktm May 14, 2024

Choose a reason for hiding this comment

legoktm May 14, 2024 • edited

Choose a reason for hiding this comment

rocodes May 10, 2024 •

edited

legoktm May 14, 2024 •

edited

rocodes commented May 10, 2024 •

edited

legoktm May 14, 2024 •

edited