*: Always pass check= argument to subprocess.run()

- Avoid flake8 warnings.

- Makes the call more explicitly readable in case an exception is expected but
check=True is not passed by mistake.

Tests:

- Many tests are skipped since the changes are considered trivial.
check=False is already the default for subprocess.run() method.

- actions/package: Install an app when it is not installed.

- actions/upgrade: Run manual upgrades.

- actions/users: Change a user password. Login. Create/remove a user.

- actions/zoph: Restore a database.

- container: On a fresh repository, run ./container up,ssh,stop,destroy for a
testing container.

- plinth/action_utils.py: Enable/disable an app that has a running service.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

actions/monkeysphere

@@ -226,7 +226,8 @@ def _get_ssh_key_file_for_import(original_key_file, service):
         shutil.copy2(original_key_file, key_file)
         # Convert OpenSSH format to PEM
         subprocess.run(
-            ['ssh-keygen', '-p', '-N', '', '-m', 'PEM', '-f', key_file])
+            ['ssh-keygen', '-p', '-N', '', '-m', 'PEM', '-f', key_file],
+            check=True)
         yield key_file
 
 

actions/packages

@@ -96,7 +96,7 @@ def subcommand_install(arguments):
     if arguments.force_missing_configuration:
         extra_arguments += ['-o', 'Dpkg::Options::=--force-confmiss']
 
-    subprocess.run(['dpkg', '--configure', '-a'])
+    subprocess.run(['dpkg', '--configure', '-a'], check=False)
     with apt_hold_freedombox():
         run_apt_command(['--fix-broken', 'install'])
         returncode = run_apt_command(['install'] + extra_arguments +

actions/storage

@@ -273,7 +273,7 @@ def subcommand_mount(arguments):
     process = subprocess.run([
         'udisksctl', 'mount', '--block-device', arguments.block_device,
         '--no-user-interaction'
-    ])
+    ], check=False)
     sys.exit(process.returncode)
 
 

actions/upgrades

@@ -140,7 +140,7 @@ def parse_arguments():
 
 def _run():
     """Run unattended-upgrades"""
-    subprocess.run(['dpkg', '--configure', '-a'])
+    subprocess.run(['dpkg', '--configure', '-a'], check=False)
     run_apt_command(['--fix-broken', 'install'])
 
     # In case freedombox package was left in held state by an
@@ -453,7 +453,7 @@ def _perform_dist_upgrade():
             ['grub-pc grub-pc/install_devices_empty boolean true'])
 
         print('Running unattended-upgrade...', flush=True)
-        subprocess.run(['unattended-upgrade', '--verbose'])
+        subprocess.run(['unattended-upgrade', '--verbose'], check=False)
 
         # Remove obsolete packages that may prevent other packages from
         # upgrading.
@@ -492,7 +492,7 @@ def _perform_dist_upgrade():
     # Run unattended-upgrade once more to handle upgrading the
     # freedombox package.
     print('Running unattended-upgrade...', flush=True)
-    subprocess.run(['unattended-upgrade', '--verbose'])
+    subprocess.run(['unattended-upgrade', '--verbose'], check=False)
 
     # Restore original snapshots configuration.
     if snapshots_supported and apt_snapshots_enabled:

actions/users

@@ -373,7 +373,7 @@ def set_samba_user(username, password):
     """
     proc = subprocess.run(['smbpasswd', '-a', '-s', username],
                           input='{0}\n{0}\n'.format(password).encode(),
-                          stderr=subprocess.PIPE)
+                          stderr=subprocess.PIPE, check=False)
     if proc.returncode != 0:
         raise RuntimeError('Unable to add Samba user: ', proc.stderr)
 
@@ -580,13 +580,12 @@ def flush_cache():
     action_utils.service_reload('apache2')
 
 
-def _run(arguments, **kwargs):
+def _run(arguments, check=True, **kwargs):
     """Run a command. Check return code and suppress output by default."""
     env = dict(os.environ, LDAPSCRIPTS_CONF=LDAPSCRIPTS_CONF)
     kwargs['stdout'] = kwargs.get('stdout', subprocess.DEVNULL)
     kwargs['stderr'] = kwargs.get('stderr', subprocess.DEVNULL)
-    kwargs['check'] = kwargs.get('check', True)
+    return subprocess.run(arguments, env=env, check=check, **kwargs)
-    return subprocess.run(arguments, env=env, **kwargs)
 
 
 def main():

actions/zoph

@@ -128,7 +128,7 @@ def subcommand_dump_database(_):
 def subcommand_restore_database(_):
     """Restore database from file."""
     db_name = _get_db_name()
-    subprocess.run(['mysqladmin', '--force', 'drop', db_name])
+    subprocess.run(['mysqladmin', '--force', 'drop', db_name], check=False)
     subprocess.run(['mysqladmin', 'create', db_name], check=True)
     with open(DB_BACKUP_FILE, 'r') as db_restore_file:
         subprocess.run(['mysql', db_name], stdin=db_restore_file, check=True)

container

@@ -327,7 +327,7 @@ def _check_command(command):
     else:
         which = ['sudo', 'which', command]
 
-    process = subprocess.run(which, stdout=subprocess.DEVNULL)
+    process = subprocess.run(which, stdout=subprocess.DEVNULL, check=False)
     return process.returncode == 0
 
 
@@ -341,7 +341,8 @@ def _verify_dependencies():
     # it leading to machinectl start failing first time after boot. See
     # https://github.com/systemd/systemd/issues/13130 . Workaround for old
     # versions of machinectl. Ignore errors.
-    subprocess.run(['sudo', 'modprobe', '--all', '--quiet', 'loop'])
+    subprocess.run(['sudo', 'modprobe', '--all', '--quiet', 'loop'],
+                   check=False)
 
     dependencies = {
         'systemd-nspawn': 'systemd-container',
@@ -370,13 +371,13 @@ def _verify_dependencies():
                 ' '.join(missing_commands))
 
     process = subprocess.run(['lsb_release', '--id', '--short'],
-                             stdout=subprocess.PIPE)
+                             stdout=subprocess.PIPE, check=False)
     if process.stdout.decode().strip() != 'Debian':
         sys.exit(1)
 
     logger.info('Running apt for missing packages: %s',
                 ' '.join(missing_commands))
-    subprocess.run(['sudo', 'apt', 'install'] + missing_packages)
+    subprocess.run(['sudo', 'apt', 'install'] + missing_packages, check=False)
 
 
 def _get_systemd_nspawn_version():
@@ -583,7 +584,7 @@ def _setup_nm_connection(distribution):
     connection_name = f'fbx-{distribution}-shared'
     process = subprocess.run(
         ['sudo', 'nmcli', 'connection', 'show', connection_name],
-        stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+        stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False)
     if not process.returncode:
         return
 
@@ -600,7 +601,8 @@ def _setup_nm_connection(distribution):
     subprocess.run(['sudo', 'nmcli', 'connection', 'add'] +
                    list(itertools.chain(*properties.items())), check=True)
     subprocess.run(['sudo', 'nmcli', 'connection', 'up', connection_name],
-                   stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+                   stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
+                   check=False)
 
 
 def _setup_users(image_file):
@@ -649,7 +651,7 @@ def _setup_ssh(image_file):
         logger.info('Generating SSH client key %s', key_file)
         subprocess.run(
             ['ssh-keygen', '-t', 'ed25519', '-N', '', '-f',
-             str(key_file)], stdout=subprocess.DEVNULL)
+             str(key_file)], stdout=subprocess.DEVNULL, check=True)
 
     public_key_file = key_file.with_suffix('.pub')
     public_key = public_key_file.read_bytes()
@@ -713,33 +715,35 @@ VirtualEthernet=yes
 '''
     nspawn_file = f'/run/systemd/nspawn/{machine_name}.nspawn'
     logger.info('Creating systemd-nspawn configuration: %s', nspawn_file)
-    subprocess.run(['sudo', 'rm', '--force', nspawn_file])
+    subprocess.run(['sudo', 'rm', '--force', nspawn_file], check=False)
     subprocess.run(['sudo', 'tee', nspawn_file], input=nspawn_options.encode(),
                    stdout=subprocess.DEVNULL, check=True)
 
     image_link = pathlib.Path(f'/var/lib/machines/{machine_name}.raw')
     logger.info('Linking systemd-nspawn image %s -> %s', image_link,
                 image_file)
-    result = subprocess.run(['sudo', 'test', '-e', str(image_link)])
+    result = subprocess.run(
+        ['sudo', 'test', '-e', str(image_link)], check=False)
     if not result.returncode:
-        result = subprocess.run(['sudo', 'test', '-L', str(image_link)])
+        result = subprocess.run(
+            ['sudo', 'test', '-L', str(image_link)], check=False)
         if result.returncode:
             raise Exception(f'Image file {image_link} is not a symlink.')
 
-        subprocess.run(['sudo', 'rm', '--force', str(image_link)])
+        subprocess.run(['sudo', 'rm', '--force', str(image_link)], check=False)
 
     subprocess.run([
         'sudo', 'ln', '--symbolic',
         str(image_file.resolve()),
         str(image_link)
-    ])
+    ], check=False)
 
 
 def _get_machine_status(machine_name):
     """Return the running status of a container."""
     process = subprocess.run(['sudo', 'machinectl', 'status', machine_name],
                              stdout=subprocess.DEVNULL,
-                             stderr=subprocess.DEVNULL)
+                             stderr=subprocess.DEVNULL, check=False)
     return process.returncode == 0
 
 
@@ -756,14 +760,14 @@ def _launch(image_file, distribution):
                 f'fbx-{distribution}-shared')
     subprocess.run(
         ['sudo', 'nmcli', 'connection', 'up', f'fbx-{distribution}-shared'],
-        stdout=subprocess.DEVNULL)
+        stdout=subprocess.DEVNULL, check=False)
 
 
 def _stop(distribution):
     """Stop the container."""
     machine_name = f'fbx-{distribution}'
     logger.info('Running `machinectl stop %s`', machine_name)
-    subprocess.run(['sudo', 'machinectl', 'stop', machine_name])
+    subprocess.run(['sudo', 'machinectl', 'stop', machine_name], check=False)
     _wait_for(lambda: not _get_machine_status(machine_name))
 
 
@@ -771,7 +775,8 @@ def _terminate(distribution):
     """Terminal the container."""
     machine_name = f'fbx-{distribution}'
     logger.info('Running `machinectl terminate %s`', machine_name)
-    subprocess.run(['sudo', 'machinectl', 'terminate', machine_name])
+    subprocess.run(['sudo', 'machinectl', 'terminate', machine_name],
+                   check=False)
     _wait_for(lambda: not _get_machine_status(machine_name))
 
 
@@ -781,16 +786,17 @@ def _destroy(distribution):
     image_link = pathlib.Path(f'/var/lib/machines/{machine_name}.raw')
 
     logger.info('Removing link to systemd-nspawn image %s', image_link)
-    subprocess.run(['sudo', 'rm', '--force', str(image_link)])
+    subprocess.run(['sudo', 'rm', '--force', str(image_link)], check=False)
     nspawn_file = f'/run/systemd/nspawn/{machine_name}.nspawn'
 
     logger.info('Removing systemd-nspawn configuration: %s', nspawn_file)
-    subprocess.run(['sudo', 'rm', '--force', nspawn_file])
+    subprocess.run(['sudo', 'rm', '--force', nspawn_file], check=False)
 
     overlay_folder = _get_overlay_folder(distribution)
     logger.info('Removing overlay folder with container written data: %s',
                 overlay_folder)
-    subprocess.run(['sudo', 'rm', '-r', '--force', overlay_folder])
+    subprocess.run(['sudo', 'rm', '-r', '--force', overlay_folder],
+                   check=False)
 
     compressed_image = _get_compressed_image_path(distribution)
     image_file = compressed_image.with_suffix('')
@@ -814,7 +820,7 @@ def _destroy(distribution):
     logger.info('Removing Network Manager connection %s', connection_name)
     result = subprocess.run(
         ['sudo', 'nmcli', 'connection', 'delete', connection_name],
-        capture_output=True)
+        capture_output=True, check=False)
     if result.returncode not in (0, 10):
         # nmcli failed and not due to 'Connection, device, or access point does
         # not exist.' See

plinth/action_utils.py

@@ -128,10 +128,10 @@ def service_action(service_name, action):
     """Perform the given action on the service_name."""
     if is_systemd_running():
         subprocess.run(['systemctl', action, service_name],
-                       stdout=subprocess.DEVNULL)
+                       stdout=subprocess.DEVNULL, check=False)
     else:
         subprocess.run(['service', service_name, action],
-                       stdout=subprocess.DEVNULL)
+                       stdout=subprocess.DEVNULL, check=False)
 
 
 def webserver_is_enabled(name, kind='config'):
@@ -371,7 +371,7 @@ Owners: {package}
     env['DEBCONF_DB_OVERRIDE'] = 'File{' + override_file.name + \
                                  ' readonly:true}'
     env['DEBIAN_FRONTEND'] = 'noninteractive'
-    subprocess.run(['dpkg-reconfigure', package], env=env)
+    subprocess.run(['dpkg-reconfigure', package], env=env, check=False)
 
     try:
         os.remove(override_file.name)
@@ -419,7 +419,7 @@ def run_apt_command(arguments):
     env['DEBIAN_FRONTEND'] = 'noninteractive'
     process = subprocess.run(command, stdin=subprocess.DEVNULL,
                              stdout=subprocess.DEVNULL, close_fds=False,
-                             env=env)
+                             env=env, check=False)
     return process.returncode
 
 
@@ -458,7 +458,8 @@ def apt_hold_freedombox():
 def apt_unhold_freedombox():
     """Remove any hold on freedombox package, and clear flag."""
     subprocess.run(['apt-mark', 'unhold', 'freedombox'],
-                   stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+                   stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
+                   check=False)
     if apt_hold_flag.exists():
         apt_hold_flag.unlink()
 

plinth/modules/backups/forms.py

@@ -285,12 +285,13 @@ class VerifySshHostkeyForm(forms.Form):
         # Fetch public keys of ssh remote
         keyscan = subprocess.run(['ssh-keyscan', hostname],
                                  stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE)
+                                 stderr=subprocess.PIPE, check=False)
         keys = keyscan.stdout.decode().splitlines()
         error_message = keyscan.stderr.decode() if keyscan.returncode else None
         # Generate user-friendly fingerprints of public keys
         keygen = subprocess.run(['ssh-keygen', '-l', '-f', '-'],
-                                input=keyscan.stdout, stdout=subprocess.PIPE)
+                                input=keyscan.stdout, stdout=subprocess.PIPE,
+                                check=False)
         fingerprints = keygen.stdout.decode().splitlines()
 
         return zip(keys, fingerprints), error_message

plinth/modules/backups/tests/test_ssh_remotes.py

@@ -38,7 +38,7 @@ def fixture_password():
 
 def get_hashed_password(password):
     res = subprocess.run(['mkpasswd', '--method=md5', password],
-                         stdout=subprocess.PIPE)
+                         stdout=subprocess.PIPE, check=True)
     return res.stdout.decode().strip()
 
 

plinth/modules/email_server/audit/domain.py

@@ -244,8 +244,8 @@ def _action_set_keys():
         clean_dict[key] = clean_function(value)
 
     # Apply changes (postconf)
-    postconf_dict = dict(filter(lambda kv: not kv[0].startswith('_'),
+    postconf_dict = dict(
-                                clean_dict.items()))
+        filter(lambda kv: not kv[0].startswith('_'), clean_dict.items()))
     postconf.set_many(postconf_dict)
 
     # Apply changes (special)
@@ -258,7 +258,7 @@ def _action_set_keys():
     with postconf.mutex.lock_all():
         # systemctl reload postfix
         args = ['systemctl', 'reload', 'postfix']
-        completed = subprocess.run(args, capture_output=True)
+        completed = subprocess.run(args, capture_output=True, check=False)
         if completed.returncode != 0:
             interproc.log_subprocess(completed)
             raise OSError('Could not reload postfix')

plinth/modules/email_server/audit/home.py

@@ -65,7 +65,7 @@ def action_mk(arg_type, user_info):
 
     args = ['sudo', '-n', '--user=#' + str(passwd.pw_uid)]
     args.extend(['/bin/sh', '-c', 'mkdir -p ~'])
-    completed = subprocess.run(args, capture_output=True)
+    completed = subprocess.run(args, capture_output=True, check=False)
     if completed.returncode != 0:
         interproc.log_subprocess(completed)
         raise OSError('Could not create home directory')

plinth/modules/email_server/audit/spam.py

@@ -140,7 +140,7 @@ def _compile_sieve():
 def _run_sievec(sieve_file):
     logger.info('Compiling sieve script %s', sieve_file)
     args = ['sievec', '--', sieve_file]
-    completed = subprocess.run(args, capture_output=True)
+    completed = subprocess.run(args, capture_output=True, check=False)
     if completed.returncode != 0:
         interproc.log_subprocess(completed)
         raise OSError('Sieve compilation failed: ' + sieve_file)

plinth/modules/email_server/lock.py

@@ -89,7 +89,7 @@ class Mutex:
         args.extend(['/bin/sh', '-c'])
         args.append('umask 177 && > ' + self.lock_path)
 
-        completed = subprocess.run(args, capture_output=True)
+        completed = subprocess.run(args, capture_output=True, check=False)
         if completed.returncode != 0:
             interproc.log_subprocess(completed)
             raise OSError('Could not create ' + self.lock_path)

plinth/modules/storage/tests/test_storage.py

@@ -56,7 +56,7 @@ class Disk():
         command = 'losetup --show --find {file}'.format(
             file=self.disk_file.name)
         process = subprocess.run(command.split(), stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE)
+                                 stderr=subprocess.PIPE, check=False)
         if process.returncode:
             if b'cannot find an unused loop device' in process.stderr:
                 pytest.skip('Loopback devices not available')
@@ -107,7 +107,7 @@ class Disk():
 
     def _cleanup_loopback(self):
         """Undo the loopback device setup."""
-        subprocess.run(['losetup', '--detach', self.device])
+        subprocess.run(['losetup', '--detach', self.device], check=False)
 
     def _remove_disk_file(self):
         """Delete the disk_file."""
@@ -231,16 +231,15 @@ class TestActions:
         self.assert_aligned(partition_number)
 
     @staticmethod
-    def call_action(action_command, **kwargs):
+    def call_action(action_command, check=True, **kwargs):
         """Call the action script."""
         test_directory = pathlib.Path(__file__).parent
         top_directory = (test_directory / '..' / '..' / '..' / '..').resolve()
         action_command[0] = top_directory / 'actions' / action_command[0]
         kwargs['stdout'] = kwargs.get('stdout', subprocess.DEVNULL)
         kwargs['stderr'] = kwargs.get('stderr', subprocess.DEVNULL)
-        kwargs['check'] = kwargs.get('check', True)
         env = dict(os.environ, PYTHONPATH=str(top_directory))
-        return subprocess.run(action_command, env=env, **kwargs)
+        return subprocess.run(action_command, env=env, check=check, **kwargs)
 
     def check_action(self, action_command):
         """Return success/failure result of the action command."""
@@ -255,7 +254,7 @@ class TestActions:
         subprocess.run([
             'parted', '--script', self.device, 'align-check', 'opti',
             str(partition_number)
-        ])
+        ], check=True)
 
     def assert_btrfs_file_system_healthy(self, partition_number):
         """Perform a successful ext4 file system check."""

plinth/modules/upgrades/views.py

@@ -114,7 +114,8 @@ def get_log():
 def _is_updating():
     """Check if manually triggered update is running."""
     command = ['systemctl', 'is-active', 'freedombox-manual-upgrade']
-    result = subprocess.run(command, capture_output=True, text=True)
+    result = subprocess.run(command, capture_output=True, text=True,
+                            check=False)
     return str(result.stdout).startswith('activ')  # 'active' or 'activating'
 
 

plinth/modules/users/tests/test_actions.py

@@ -138,12 +138,11 @@ def fixture_auto_cleanup_users_groups(needs_root, load_cfg):
         _delete_group(group)
 
 
-def _call_action(arguments, **kwargs):
+def _call_action(arguments, check=True, **kwargs):
     """Call the action script."""
     kwargs['stdout'] = kwargs.get('stdout', subprocess.PIPE)
     kwargs['stderr'] = kwargs.get('stderr', subprocess.PIPE)
-    kwargs['check'] = kwargs.get('check', True)
+    return subprocess.run([_action_file()] + arguments, check=check, **kwargs)
-    return subprocess.run([_action_file()] + arguments, **kwargs)
 
 
 def _create_user(username=None, groups=None):