[sonic-package-manager] code style fixes and enhancements (sonic-net#1802)

Highlights:
* Don't use f-strings where it is not needed
* Indentation fixes in comprehension expressions
* Created containers removal method in DockerApi
* Fixed a debug message where dependency instead of conflict variable
was displayed
* Return value type annotations for some functions
* Docstrings fixes & removed trailing spaces.

Signed-off-by: Stepan Blyschak <[email protected]>

Author: stepanblyschak

sonic_package_manager/constraint.py

@@ -95,7 +95,7 @@ def parse(constraints: Dict) -> 'ComponentConstraints':
         """
 
         components = {component: VersionConstraint.parse(version)
-                     for component, version in constraints.items()}
+                      for component, version in constraints.items()}
         return ComponentConstraints(components)
 
     def deparse(self) -> Dict[str, str]:

sonic_package_manager/dockerapi.py

@@ -186,6 +186,15 @@ def rm(self, container: str, **kwargs):
         self.client.containers.get(container).remove(**kwargs)
         log.debug(f'removed container {container}')
 
+    def rm_by_ancestor(self, image_id: str, **kwargs):
+        """ Docker 'rm' command for running containers instantiated
+        from image passed to this function. """
+
+        # Clean containers based on the old image
+        containers = self.ps(filters={'ancestor': image_id}, all=True)
+        for container in containers:
+            self.rm(container.id, **kwargs)
+
     def ps(self, **kwargs):
         """ Docker 'ps' command. """
 

sonic_package_manager/errors.py

@@ -143,4 +143,3 @@ class PackageComponentConflictError(PackageInstallationError):
     def __str__(self):
         return (f'Package {self.name} conflicts with {self.component} {self.constraint} '
                 f'in package {self.dependency} but version {self.installed_ver} is installed')
-

sonic_package_manager/main.py

@@ -361,7 +361,7 @@ def install(ctx,
 
     package_source = package_expr or from_repository or from_tarball
     if not package_source:
-        exit_cli(f'Package source is not specified', fg='red')
+        exit_cli('Package source is not specified', fg='red')
 
     if not yes and not force:
         click.confirm(f'{package_source} is going to be installed, '
@@ -386,7 +386,7 @@ def install(ctx,
     except Exception as err:
         exit_cli(f'Failed to install {package_source}: {err}', fg='red')
     except KeyboardInterrupt:
-        exit_cli(f'Operation canceled by user', fg='red')
+        exit_cli('Operation canceled by user', fg='red')
 
 
 @cli.command()
@@ -409,7 +409,7 @@ def reset(ctx, name, force, yes, skip_host_plugins):
     except Exception as err:
         exit_cli(f'Failed to reset package {name}: {err}', fg='red')
     except KeyboardInterrupt:
-        exit_cli(f'Operation canceled by user', fg='red')
+        exit_cli('Operation canceled by user', fg='red')
 
 
 @cli.command()
@@ -426,12 +426,16 @@ def uninstall(ctx, name, force, yes):
         click.confirm(f'Package {name} is going to be uninstalled, '
                       f'continue?', abort=True, show_default=True)
 
+    uninstall_opts = {
+        'force': force,
+    }
+
     try:
-        manager.uninstall(name, force)
+        manager.uninstall(name, **uninstall_opts)
     except Exception as err:
         exit_cli(f'Failed to uninstall package {name}: {err}', fg='red')
     except KeyboardInterrupt:
-        exit_cli(f'Operation canceled by user', fg='red')
+        exit_cli('Operation canceled by user', fg='red')
 
 
 @cli.command()
@@ -453,7 +457,7 @@ def migrate(ctx, database, force, yes, dockerd_socket):
     except Exception as err:
         exit_cli(f'Failed to migrate packages {err}', fg='red')
     except KeyboardInterrupt:
-        exit_cli(f'Operation canceled by user', fg='red')
+        exit_cli('Operation canceled by user', fg='red')
 
 
 if __name__ == "__main__":

sonic_package_manager/manager.py

@@ -39,6 +39,7 @@
 from sonic_package_manager.progress import ProgressManager
 from sonic_package_manager.reference import PackageReference
 from sonic_package_manager.registry import RegistryResolver
+from sonic_package_manager.service_creator import SONIC_CLI_COMMANDS
 from sonic_package_manager.service_creator.creator import (
     ServiceCreator,
     run_command
@@ -52,7 +53,6 @@
     RegistrySource,
     TarballSource
 )
-from sonic_package_manager.utils import DockerReference
 from sonic_package_manager.version import (
     Version,
     version_to_tag,
@@ -101,7 +101,7 @@ def wrapped_function(*args, **kwargs):
     return wrapped_function
 
 
-def rollback(func, *args, **kwargs):
+def rollback(func, *args, **kwargs) -> Callable:
     """ Used in rollback callbacks to ignore failure
     but proceed with rollback. Error will be printed
     but not fail the whole procedure of rollback. """
@@ -131,7 +131,7 @@ def package_constraint_to_reference(constraint: PackageConstraint) -> PackageRef
     return PackageReference(package_name, version_to_tag(version))
 
 
-def parse_reference_expression(expression):
+def parse_reference_expression(expression) -> PackageReference:
     try:
         return package_constraint_to_reference(PackageConstraint.parse(expression))
     except ValueError:
@@ -247,7 +247,7 @@ def validate_package_tree(packages: Dict[str, Package]):
                     continue
 
                 component_version = conflicting_package.components[component]
-                log.debug(f'conflicting package {dependency.name}: '
+                log.debug(f'conflicting package {conflict.name}: '
                           f'component {component} version is {component_version}')
 
                 if constraint.allows(component_version):
@@ -397,12 +397,17 @@ def install_from_source(self,
         if not self.database.has_package(package.name):
             self.database.add_package(package.name, package.repository)
 
+        service_create_opts = {
+            'state': feature_state,
+            'owner': default_owner,
+        }
+
         try:
             with contextlib.ExitStack() as exits:
                 source.install(package)
                 exits.callback(rollback(source.uninstall, package))
 
-                self.service_creator.create(package, state=feature_state, owner=default_owner)
+                self.service_creator.create(package, **service_create_opts)
                 exits.callback(rollback(self.service_creator.remove, package))
 
                 self.service_creator.generate_shutdown_sequence_files(
@@ -481,13 +486,7 @@ def uninstall(self, name: str, force=False):
             self.service_creator.generate_shutdown_sequence_files(
                 self._get_installed_packages_except(package)
             )
-
-            # Clean containers based on this image
-            containers = self.docker.ps(filters={'ancestor': package.image_id},
-                                        all=True)
-            for container in containers:
-                self.docker.rm(container.id, force=True)
-
+            self.docker.rm_by_ancestor(package.image_id, force=True)
             self.docker.rmi(package.image_id, force=True)
             package.entry.image_id = None
         except Exception as err:
@@ -563,6 +562,13 @@ def upgrade_from_source(self,
 
         # After all checks are passed we proceed to actual upgrade
 
+        service_create_opts = {
+            'register_feature': False,
+        }
+        service_remove_opts = {
+            'deregister_feature': False,
+        }
+
         try:
             with contextlib.ExitStack() as exits:
                 self._uninstall_cli_plugins(old_package)
@@ -576,19 +582,15 @@ def upgrade_from_source(self,
                     exits.callback(rollback(self._systemctl_action,
                                             old_package, 'start'))
 
-                self.service_creator.remove(old_package, deregister_feature=False)
+                self.service_creator.remove(old_package, **service_remove_opts)
                 exits.callback(rollback(self.service_creator.create, old_package,
-                                        register_feature=False))
+                                        **service_create_opts))
 
-                # Clean containers based on the old image
-                containers = self.docker.ps(filters={'ancestor': old_package.image_id},
-                                            all=True)
-                for container in containers:
-                    self.docker.rm(container.id, force=True)
+                self.docker.rm_by_ancestor(old_package.image_id, force=True)
 
-                self.service_creator.create(new_package, register_feature=False)
+                self.service_creator.create(new_package, **service_create_opts)
                 exits.callback(rollback(self.service_creator.remove, new_package,
-                                        register_feature=False))
+                                        **service_remove_opts))
 
                 self.service_creator.generate_shutdown_sequence_files(
                     self._get_installed_packages_and(new_package)
@@ -654,16 +656,16 @@ def migrate_packages(self,
                          old_package_database: PackageDatabase,
                          dockerd_sock: Optional[str] = None):
         """
-        Migrate packages from old database. This function can do a comparison between 
-        current database and the database passed in as argument. If the package is 
-        missing in the current database it will be added. If the package is installed 
-        in the passed database and in the current it is not installed it will be 
-        installed with a passed database package version. If the package is installed 
-        in the passed database and it is installed in the current database but with 
-        older version the package will be upgraded to the never version. If the package 
-        is installed in the passed database and in the current it is installed but with 
-        never version - no actions are taken. If dockerd_sock parameter is passed, the 
-        migration process will use loaded images from docker library of the currently 
+        Migrate packages from old database. This function can do a comparison between
+        current database and the database passed in as argument. If the package is
+        missing in the current database it will be added. If the package is installed
+        in the passed database and in the current it is not installed it will be
+        installed with a passed database package version. If the package is installed
+        in the passed database and it is installed in the current database but with
+        older version the package will be upgraded to the never version. If the package
+        is installed in the passed database and in the current it is installed but with
+        never version - no actions are taken. If dockerd_sock parameter is passed, the
+        migration process will use loaded images from docker library of the currently
         installed image.
 
         Args:
@@ -784,7 +786,7 @@ def get_package_source(self,
             ref = parse_reference_expression(package_expression)
             return self.get_package_source(package_ref=ref)
         elif repository_reference:
-            repo_ref = DockerReference.parse(repository_reference)
+            repo_ref = utils.DockerReference.parse(repository_reference)
             repository = repo_ref['name']
             reference = repo_ref['tag'] or repo_ref['digest']
             reference = reference or 'latest'
@@ -815,8 +817,8 @@ def get_package_source(self,
                 if package_entry.default_reference is not None:
                     package_ref.reference = package_entry.default_reference
                 else:
-                    raise PackageManagerError(f'No default reference tag. '
-                                              f'Please specify the version or tag explicitly')
+                    raise PackageManagerError('No default reference tag. '
+                                              'Please specify the version or tag explicitly')
 
             return RegistrySource(package_entry.repository,
                                   package_ref.reference,
@@ -888,7 +890,7 @@ def get_installed_packages_list(self) -> List[Package]:
             Installed packages dictionary.
         """
 
-        return [self.get_installed_package(entry.name) 
+        return [self.get_installed_package(entry.name)
                 for entry in self.database if entry.installed]
 
     def _migrate_package_database(self, old_package_database: PackageDatabase):
@@ -948,11 +950,11 @@ def _systemctl_action(self, package: Package, action: str):
                 run_command(f'systemctl {action} {name}@{npu}')
 
     def _install_cli_plugins(self, package: Package):
-        for command in ('show', 'config', 'clear'):
+        for command in SONIC_CLI_COMMANDS:
             self._install_cli_plugin(package, command)
 
     def _uninstall_cli_plugins(self, package: Package):
-        for command in ('show', 'config', 'clear'):
+        for command in SONIC_CLI_COMMANDS:
             self._uninstall_cli_plugin(package, command)
 
     def _install_cli_plugin(self, package: Package, command: str):
@@ -978,12 +980,17 @@ def get_manager() -> 'PackageManager':
             PackageManager
         """
 
-        docker_api = DockerApi(docker.from_env())
+        docker_api = DockerApi(docker.from_env(), ProgressManager())
         registry_resolver = RegistryResolver()
-        return PackageManager(DockerApi(docker.from_env(), ProgressManager()),
+        metadata_resolver = MetadataResolver(docker_api, registry_resolver)
+        feature_registry = FeatureRegistry(SonicDB)
+        service_creator = ServiceCreator(feature_registry,
+                                         SonicDB)
+
+        return PackageManager(docker_api,
                               registry_resolver,
                               PackageDatabase.from_file(),
-                              MetadataResolver(docker_api, registry_resolver),
-                              ServiceCreator(FeatureRegistry(SonicDB), SonicDB),
+                              metadata_resolver,
+                              service_creator,
                               device_info,
                               filelock.FileLock(PACKAGE_MANAGER_LOCK_FILE, timeout=0))

sonic_package_manager/metadata.py

@@ -24,10 +24,10 @@ def deep_update(dst: Dict, src: Dict) -> Dict:
 
     for key, value in src.items():
         if isinstance(value, dict):
-             node = dst.setdefault(key, {})
-             deep_update(node, value)
+            node = dst.setdefault(key, {})
+            deep_update(node, value)
         else:
-             dst[key] = value
+            dst[key] = value
     return dst
 
 
@@ -183,3 +183,4 @@ def from_labels(cls, labels: Dict[str, str]) -> Metadata:
                     raise MetadataError(f'Failed to parse component version: {err}')
 
         return Metadata(Manifest.marshal(manifest_dict), components)
+

sonic_package_manager/registry.py

@@ -38,7 +38,7 @@ def get_token(realm, service, scope) -> str:
 
         response = requests.get(f'{realm}?scope={scope}&service={service}')
         if response.status_code != requests.codes.ok:
-            raise AuthenticationServiceError(f'Failed to retrieve token')
+            raise AuthenticationServiceError('Failed to retrieve token')
 
         content = json.loads(response.content)
         token = content['token']

sonic_package_manager/service_creator/__init__.py

@@ -1,3 +1,4 @@
 #!/usr/bin/env python
 
 ETC_SONIC_PATH = '/etc/sonic'
+SONIC_CLI_COMMANDS = ('show', 'config', 'clear')