From 295d32109bbb68c051acaa39778cbebbf5b2b47f Mon Sep 17 00:00:00 2001 From: sg893052 Date: Tue, 2 May 2023 03:02:45 -0700 Subject: [PATCH 1/4] ThirdPartyContainerManagement_in_SonicPackageManager ThirdPartyContainerManagement(TPCM) support in SonicPackageManager allows third party dockers to be installed on the sonic system. The Manifest file is generated from a local default file. The Manifest file could be updated through "sonic-package-manager manifests update" command and later the running package could be updated with the new manifest file through "sonic-package-manager update" --- sonic_package_manager/main.py | 309 ++++++++- sonic_package_manager/manager.py | 133 +++- sonic_package_manager/manifest.py | 68 ++ sonic_package_manager/metadata.py | 99 ++- .../service_creator/creator.py | 6 + sonic_package_manager/source.py | 33 +- tests/sonic_package_manager/conftest.py | 6 +- tests/sonic_package_manager/test_cli.py | 594 ++++++++++++++++++ tests/sonic_package_manager/test_manager.py | 29 + tests/sonic_package_manager/test_metadata.py | 72 ++- .../test_service_creator.py | 37 ++ 11 files changed, 1334 insertions(+), 52 deletions(-) diff --git a/sonic_package_manager/main.py b/sonic_package_manager/main.py index 8a0aabb901..4943a64f34 100644 --- a/sonic_package_manager/main.py +++ b/sonic_package_manager/main.py @@ -9,12 +9,20 @@ import click import click_log import tabulate +from urllib.parse import urlparse +import paramiko +import requests +import getpass +import shutil from natsort import natsorted from sonic_package_manager.database import PackageEntry, PackageDatabase from sonic_package_manager.errors import PackageManagerError from sonic_package_manager.logger import log from sonic_package_manager.manager import PackageManager +from sonic_package_manager.manifest import Manifest, DEFAULT_MANIFEST, MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME, DMFILE_NAME +LOCAL_TARBALL_PATH="/tmp/local_tarball.gz" +LOCAL_JSON="/tmp/local_json" BULLET_UC = '\u2022' @@ -97,11 +105,8 @@ def handle_parse_result(self, ctx, opts, args): cls=MutuallyExclusiveOption, mutually_exclusive=['from_tarball', 'package_expr']), click.option('--from-tarball', - type=click.Path(exists=True, - readable=True, - file_okay=True, - dir_okay=False), - help='Fetch package from saved image tarball.', + type=str, + help='Fetch package from saved image tarball from local/scp/sftp/http', cls=MutuallyExclusiveOption, mutually_exclusive=['from_repository', 'package_expr']), click.argument('package-expr', @@ -157,6 +162,13 @@ def repository(ctx): pass +@cli.group() +@click.pass_context +def manifests(ctx): + """ Custom local Manifest management commands. """ + + pass + @cli.group() @click.pass_context def show(ctx): @@ -215,6 +227,11 @@ def manifest(ctx, manager: PackageManager = ctx.obj try: + if from_tarball: + #Download the tar file from local/scp/sftp/http + download_file(from_tarball, LOCAL_TARBALL_PATH) + from_tarball = LOCAL_TARBALL_PATH + source = manager.get_package_source(package_expr, from_repository, from_tarball) @@ -255,6 +272,11 @@ def changelog(ctx, manager: PackageManager = ctx.obj try: + if from_tarball: + #Download the tar file from local/scp/sftp/http + download_file(from_tarball, LOCAL_TARBALL_PATH) + from_tarball = LOCAL_TARBALL_PATH + source = manager.get_package_source(package_expr, from_repository, from_tarball) @@ -280,6 +302,167 @@ def changelog(ctx, exit_cli(f'Failed to print package changelog: {err}', fg='red') + +@manifests.command('create') +@click.pass_context +@click.argument('name', type=click.Path()) +@click.option('--from-json', type=str, help='specify manifest json file') +@root_privileges_required +def create(ctx, name, from_json): + """Create a new custom local manifest file.""" + + #Validation checks + manager: PackageManager = ctx.obj + if manager.is_installed(name): + click.echo("Error: A package with the same name {} is already installed".format(name)) + return + MFILE_NAME = os.path.join(MANIFEST_LOCATION, name) + if os.path.exists(MFILE_NAME): + click.echo("Error: Manifest file '{}' already exists.".format(name)) + return + + #Creation of default manifest file in case the file does not exist + if not os.path.exists(MANIFEST_LOCATION): + os.mkdir(MANIFEST_LOCATION) + if not os.path.exists(DMFILE_NAME): + with open(DMFILE_NAME, 'w') as file: + json.dump(DEFAULT_MANIFEST, file, indent=4) + #click.echo(f"Manifest '{DEFAUT_MANIFEST_NAME}' created now.") + + + #Create the manifest file in centralized location + #Download the json file from scp/sftp/http to local_json_file + try: + if from_json: + download_file(from_json, LOCAL_JSON) + from_json = LOCAL_JSON + data = {} + with open(from_json, 'r') as file: + data = json.load(file) + #Validate with manifest scheme + Manifest.marshal(data) + + #Make sure the 'name' is overwritten into the dict + data['package']['name'] = name + data['service']['name'] = name + + with open(MFILE_NAME, 'w') as file: + json.dump(data, file, indent=4) + else: + shutil.copy(DMFILE_NAME, MFILE_NAME) + click.echo(f"Manifest '{name}' created successfully.") + except Exception as e: + click.echo("Error: Manifest {} creation failed - {}".format(name, str(e))) + return + + + +#At the end of sonic-package-manager install, a new manifest file is created with the name. +#At the end of sonic-package-manager uninstall name, this manifest file name and name.edit will be deleted. +#At the end of sonic-package-manager update, we need to mv maniests name.edit to name in case of success, else keep it as such. +#So during sonic-package-manager update, we could take old package from name and new package from edit and at the end, follow 3rd point +@manifests.command('update') +@click.pass_context +@click.argument('name', type=click.Path()) +@click.option('--from-json', type=str, required=True) +#@click.argument('--from-json', type=str, help='Specify Manifest json file') +@root_privileges_required +def update(ctx, name, from_json): + """Update an existing custom local manifest file with new one.""" + + manager: PackageManager = ctx.obj + ORG_FILE = os.path.join(MANIFEST_LOCATION, name) + if not os.path.exists(ORG_FILE): + click.echo(f'Local Manifest file for {name} does not exists to update') + return + try: + #download json file from remote/local path + download_file(from_json, LOCAL_JSON) + from_json = LOCAL_JSON + with open(from_json, 'r') as file: + data = json.load(file) + + #Validate with manifest scheme + Manifest.marshal(data) + + #Make sure the 'name' is overwritten into the dict + data['package']['name'] = name + data['service']['name'] = name + + if manager.is_installed(name): + edit_name = name + '.edit' + EDIT_FILE = os.path.join(MANIFEST_LOCATION, edit_name) + with open(EDIT_FILE, 'w') as edit_file: + json.dump(data, edit_file, indent=4) + click.echo(f"Manifest '{name}' updated successfully.") + else: + #If package is not installed, + ## update the name file directly + with open(ORG_FILE, 'w') as orig_file: + json.dump(data, orig_file, indent=4) + click.echo(f"Manifest '{name}' updated successfully.") + except Exception as e: + click.echo(f"Error occurred while updating manifest '{name}': {e}") + return + + +@manifests.command('delete') +@click.pass_context +@click.argument('name', type=click.Path()) +@root_privileges_required +def delete(ctx, name): + """Delete a custom local manifest file.""" + # Check if the manifest file exists + mfile_name = "{}{}".format(MANIFEST_LOCATION, name) + if not os.path.exists(mfile_name): + click.echo("Error: Manifest file '{}' not found.".format(name)) + return + + try: + # Confirm deletion with user input + confirm = click.prompt("Are you sure you want to delete the manifest file '{}'? (y/n)".format(name), type=str) + if confirm.lower() == 'y': + os.remove(mfile_name) + click.echo("Manifest '{}' deleted successfully.".format(name)) + else: + click.echo("Deletion cancelled.") + except Exception as e: + click.echo("Error: Failed to delete manifest file '{}'. {}".format(name, e)) + + +@manifests.command('show') +@click.pass_context +@click.argument('name', type=click.Path()) +@root_privileges_required +def show_manifest(ctx, name): + """Show the contents of custom local manifest file.""" + mfile_name = "{}{}".format(MANIFEST_LOCATION, name) + edit_file_name = "{}.edit".format(mfile_name) + try: + if os.path.exists(edit_file_name): + mfile_name = edit_file_name + with open(mfile_name, 'r') as file: + data = json.load(file) + click.echo("Manifest file: {}".format(name)) + click.echo(json.dumps(data, indent=4)) + except FileNotFoundError: + click.echo("Manifest file '{}' not found.".format(name)) + +@manifests.command('list') +@click.pass_context +@root_privileges_required +def list_manifests(ctx): + """List all custom local manifest files.""" + # Get all files in the manifest location + manifest_files = os.listdir(MANIFEST_LOCATION) + if not manifest_files: + click.echo("No custom local manifest files found.") + else: + click.echo("Custom Local Manifest files:") + for file in manifest_files: + click.echo("- {}".format(file)) + + @repository.command() @click.argument('name', type=str) @click.argument('repository', type=str) @@ -316,6 +499,78 @@ def remove(ctx, name): exit_cli(f'Failed to remove repository {name}: {err}', fg='red') +def parse_url(url): + # Parse information from URL + parsed_url = urlparse(url) + if parsed_url.scheme == "scp" or parsed_url.scheme == "sftp": + return parsed_url.username, parsed_url.password, parsed_url.hostname, parsed_url.path + elif parsed_url.scheme == "http": + return None, None, parsed_url.netloc, parsed_url.path + elif not parsed_url.scheme: # No scheme indicates a local file path + return None, None, None, parsed_url.path + else: + raise ValueError("Unsupported URL scheme") + +def validate_url_or_abort(url): + # Attempt to retrieve HTTP response code + try: + response = requests.head(url) + response_code = response.status_code + except requests.exceptions.RequestException as err: + response_code = None + + if not response_code: + print("Did not receive a response from remote machine. Aborting...") + return + else: + # Check for a 4xx response code which indicates a nonexistent URL + if str(response_code).startswith('4'): + print("Image file not found on remote machine. Aborting...") + return + +def download_file(url, local_path): + # Parse information from the URL + username, password, hostname, remote_path = parse_url(url) + + if username is not None: + # If password is not provided, prompt the user for it securely + if password is None: + password = getpass.getpass(prompt=f"Enter password for {username}@{hostname}: ") + + # Create an SSH client for SCP or SFTP + client = paramiko.SSHClient() + # Automatically add the server's host key (this is insecure and should be handled differently in production) + client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) + + try: + # Connect to the SSH server + client.connect(hostname, username=username, password=password) + + # Open an SCP channel for SCP or an SFTP channel for SFTP + with client.open_sftp() as sftp: + # Download the file + sftp.get(remote_path, local_path) + + finally: + # Close the SSH connection + client.close() + elif hostname: + # Download using HTTP for URLs without credentials + validate_url_or_abort(url) + try: + response = requests.get(url) + with open(local_path, 'wb') as f: + f.write(response.content) + except requests.exceptions.RequestException as e: + print("Download error", e) + return + else: + if os.path.exists(remote_path): + shutil.copy(remote_path, local_path) + else: + print(f"Error: Source file '{remote_path}' does not exist.") + + @cli.command() @click.option('--enable', is_flag=True, @@ -334,6 +589,13 @@ def remove(ctx, name): help='Allow package downgrade. By default an attempt to downgrade the package ' 'will result in a failure since downgrade might not be supported by the package, ' 'thus requires explicit request from the user.') +@click.option('--use-local-manifest', + is_flag=True, + default=None, + help='Use locally created custom manifest file ') +@click.option('--name', + type=str, + help='custom name for the package') @add_options(PACKAGE_SOURCE_OPTIONS) @add_options(PACKAGE_COMMON_OPERATION_OPTIONS) @add_options(PACKAGE_COMMON_INSTALL_OPTIONS) @@ -348,7 +610,9 @@ def install(ctx, enable, set_owner, skip_host_plugins, - allow_downgrade): + allow_downgrade, + use_local_manifest, + name): """ Install/Upgrade package using [PACKAGE_EXPR] in format "[=|@]". The repository to pull the package from is resolved by lookup in package database, @@ -378,10 +642,26 @@ def install(ctx, if allow_downgrade is not None: install_opts['allow_downgrade'] = allow_downgrade + if use_local_manifest: + if not name: + click.echo(f'name argument is not provided to use local manifest') + return + ORG_FILE = os.path.join(MANIFEST_LOCATION, name) + if not os.path.exists(ORG_FILE): + click.echo(f'Local Manifest file for {name} does not exists to install') + return + + if from_tarball: + #Download the tar file from local/scp/sftp/http + download_file(from_tarball, LOCAL_TARBALL_PATH) + from_tarball = LOCAL_TARBALL_PATH + try: manager.install(package_expr, from_repository, from_tarball, + use_local_manifest, + name, **install_opts) except Exception as err: exit_cli(f'Failed to install {package_source}: {err}', fg='red') @@ -389,6 +669,23 @@ def install(ctx, exit_cli('Operation canceled by user', fg='red') +@cli.command() +@add_options(PACKAGE_COMMON_OPERATION_OPTIONS) +@click.argument('name') +@click.pass_context +@root_privileges_required +def update(ctx, name, force, yes): + """ Update package to the updated manifest file """ + + manager: PackageManager = ctx.obj + + try: + manager.update(name, force) + except Exception as err: + exit_cli(f'Failed to update package {name}: {err}', fg='red') + except KeyboardInterrupt: + exit_cli('Operation canceled by user', fg='red') + @cli.command() @add_options(PACKAGE_COMMON_OPERATION_OPTIONS) @add_options(PACKAGE_COMMON_INSTALL_OPTIONS) diff --git a/sonic_package_manager/manager.py b/sonic_package_manager/manager.py index e41bb00e8f..7832760073 100644 --- a/sonic_package_manager/manager.py +++ b/sonic_package_manager/manager.py @@ -65,7 +65,8 @@ version_to_tag, tag_to_version ) - +from sonic_package_manager.manifest import MANIFEST_LOCATION +LOCAL_TARBALL_PATH="/tmp/local_tarball.gz" @contextlib.contextmanager def failure_ignore(ignore: bool): @@ -344,6 +345,8 @@ def install(self, expression: Optional[str] = None, repotag: Optional[str] = None, tarball: Optional[str] = None, + use_local_manifest: bool = False, + name: Optional[str] = None, **kwargs): """ Install/Upgrade SONiC Package from either an expression representing the package and its version, repository and tag or @@ -358,7 +361,7 @@ def install(self, PackageManagerError """ - source = self.get_package_source(expression, repotag, tarball) + source = self.get_package_source(expression, repotag, tarball, use_local_manifest=use_local_manifest, name=name) package = source.get_package() if self.is_installed(package.name): @@ -430,7 +433,7 @@ def install_from_source(self, self.service_creator.generate_shutdown_sequence_files, self.get_installed_packages()) ) - + if not skip_host_plugins: self._install_cli_plugins(package) exits.callback(rollback(self._uninstall_cli_plugins, package)) @@ -446,6 +449,74 @@ def install_from_source(self, self.database.update_package(package.entry) self.database.commit() + @under_lock + @opt_check + def update(self, name: str, + force: bool = False): + """ Update SONiC Package referenced by name. The update + can be forced if force argument is True. + + Args: + name: SONiC Package name. + use_local_manifest: Use this manifest to update + force: Force the installation. + Raises: + PackageManagerError + """ + + with failure_ignore(force): + if not self.is_installed(name): + raise PackageUninstallationError(f'{name} is not installed') + + old_package = self.get_installed_package(name) + new_package = self.get_installed_package(name, use_edit=True) + + service_create_opts = { + 'register_feature': False, + } + service_remove_opts = { + 'deregister_feature': False, + } + + try: + with contextlib.ExitStack() as exits: + self.service_creator.remove(old_package, **service_remove_opts) + exits.callback(rollback(self.service_creator.create, old_package, + **service_create_opts)) + + self.docker.rm_by_ancestor(old_package.image_id, force=True) + + self.service_creator.create(new_package, **service_create_opts) + exits.callback(rollback(self.service_creator.remove, new_package, + **service_remove_opts)) + + self.service_creator.generate_shutdown_sequence_files( + self._get_installed_packages_and(new_package) + ) + exits.callback(rollback( + self.service_creator.generate_shutdown_sequence_files, + self._get_installed_packages_and(old_package)) + ) + + self.feature_registry.update(old_package.manifest, new_package.manifest) + exits.callback(rollback( + self.feature_registry.update, new_package.manifest, old_package.manifest) + ) + + exits.pop_all() + except Exception as err: + raise PackageUpgradeError(f'Failed to update {new_package.name}: {err}') + except KeyboardInterrupt: + raise + + new_package_entry = new_package.entry + new_package_entry.installed = True + self.database.update_package(new_package_entry) + self.database.commit() + manifest_path = os.path.join(MANIFEST_LOCATION, name) + edit_path = os.path.join(MANIFEST_LOCATION, name + ".edit") + os.rename(edit_path,manifest_path) + @under_lock @opt_check def uninstall(self, name: str, @@ -469,6 +540,9 @@ def uninstall(self, name: str, package = self.get_installed_package(name) service_name = package.manifest['service']['name'] + image_id_used = False + image_id_used = any(entry.image_id == package.image_id for entry in self.database if entry.name != package.name) + with failure_ignore(force): if self.feature_registry.is_feature_enabled(service_name): raise PackageUninstallationError( @@ -493,7 +567,11 @@ def uninstall(self, name: str, self._get_installed_packages_except(package) ) self.docker.rm_by_ancestor(package.image_id, force=True) - self.docker.rmi(package.image_id, force=True) + # Delete image if it is not in use, otherwise skip deletion + if not image_id_used: + self.docker.rmi(package.image_id, force=True) + else: + print(f'Image with ID {package.image_id} is in use by other package(s). Skipping deletion') package.entry.image_id = None except Exception as err: raise PackageUninstallationError( @@ -504,6 +582,13 @@ def uninstall(self, name: str, package.entry.version = None self.database.update_package(package.entry) self.database.commit() + manifest_path = os.path.join(MANIFEST_LOCATION, name) + edit_path = os.path.join(MANIFEST_LOCATION, name + ".edit") + if os.path.exists(manifest_path): + os.remove(manifest_path) + if os.path.exists(edit_path): + os.remove(edit_path) + @under_lock @opt_check @@ -620,8 +705,13 @@ def upgrade_from_source(self, self._install_cli_plugins(new_package) exits.callback(rollback(self._uninstall_cli_plugin, new_package)) - self.docker.rmi(old_package.image_id, force=True) - + old_image_id_used = False + old_image_id_used = any(entry.image_id == old_package.image_id for entry in self.database if entry.name != old_package.name) + if not old_image_id_used or old_package.image_id != new_package.image_id: + self.docker.rmi(old_package.image_id, force=True) + else: + print(f'Image with ID {old_package.image_id} is in use by other package(s). Skipping deletion') + exits.pop_all() except Exception as err: raise PackageUpgradeError(f'Failed to upgrade {new_package.name}: {err}') @@ -764,7 +854,7 @@ def migrate_package(old_package_entry, self.database.commit() - def get_installed_package(self, name: str) -> Package: + def get_installed_package(self, name: str, use_local_manifest: bool = False, use_edit: bool = False) -> Package: """ Get installed package by name. Args: @@ -777,14 +867,19 @@ def get_installed_package(self, name: str) -> Package: source = LocalSource(package_entry, self.database, self.docker, - self.metadata_resolver) + self.metadata_resolver, + use_local_manifest=use_local_manifest, + name=name, + use_edit=use_edit) return source.get_package() def get_package_source(self, package_expression: Optional[str] = None, repository_reference: Optional[str] = None, tarboll_path: Optional[str] = None, - package_ref: Optional[PackageReference] = None): + package_ref: Optional[PackageReference] = None, + use_local_manifest: bool = False, + name: Optional[str] = None): """ Returns PackageSource object based on input source. Args: @@ -800,7 +895,7 @@ def get_package_source(self, if package_expression: ref = parse_reference_expression(package_expression) - return self.get_package_source(package_ref=ref) + return self.get_package_source(package_ref=ref, name=name) elif repository_reference: repo_ref = utils.DockerReference.parse(repository_reference) repository = repo_ref['name'] @@ -810,15 +905,19 @@ def get_package_source(self, reference, self.database, self.docker, - self.metadata_resolver) + self.metadata_resolver, + use_local_manifest, + name) elif tarboll_path: return TarballSource(tarboll_path, self.database, self.docker, - self.metadata_resolver) + self.metadata_resolver, + use_local_manifest, + name) elif package_ref: package_entry = self.database.get_package(package_ref.name) - + name = package_ref.name # Determine the reference if not specified. # If package is installed assume the installed # one is requested, otherwise look for default @@ -829,7 +928,9 @@ def get_package_source(self, return LocalSource(package_entry, self.database, self.docker, - self.metadata_resolver) + self.metadata_resolver, + use_local_manifest, + name) if package_entry.default_reference is not None: package_ref.reference = package_entry.default_reference else: @@ -840,7 +941,9 @@ def get_package_source(self, package_ref.reference, self.database, self.docker, - self.metadata_resolver) + self.metadata_resolver, + use_local_manifest, + name) else: raise ValueError('No package source provided') diff --git a/sonic_package_manager/manifest.py b/sonic_package_manager/manifest.py index 865db7ef5c..e87fb50776 100644 --- a/sonic_package_manager/manifest.py +++ b/sonic_package_manager/manifest.py @@ -11,6 +11,73 @@ from sonic_package_manager.errors import ManifestError from sonic_package_manager.version import Version +import os + +DEFAULT_MANIFEST = { + "version": "1.0.0", + "package": { + "version": "1.0.0", + "name": "default_manifest", + "description": "", + "base-os": {}, + "depends": [], + "breaks": [], + "init-cfg": {}, + "changelog": {}, + "debug-dump": "" + }, + "service": { + "name": "default_manifest", + "requires": [], + "requisite": [], + "wanted-by": [], + "after": [], + "before": [], + "dependent": [], + "dependent-of": [], + "post-start-action": "", + "pre-shutdown-action": "", + "asic-service": False, + "host-service": True, + "delayed": False, + "check_up_status": False, + "warm-shutdown": { + "after": [], + "before": [] + }, + "fast-shutdown": { + "after": [], + "before": [] + }, + "syslog": { + "support-rate-limit": False + } + }, + "container": { + "privileged": False, + "entrypoint": "", + "volumes": [], + "mounts": [], + "environment": {}, + "tmpfs": [] + }, + "processes": [], + "cli": { + "mandatory": False, + "show": [], + "config": [], + "clear": [], + "auto-generate-show": False, + "auto-generate-config": False, + "auto-generate-show-source-yang-modules": [], + "auto-generate-config-source-yang-modules": [] + } +} +#DEFAULT_MANIFEST = {'version': '1.0.0', 'package': {'version': '1.0.0', 'depends': [], 'name': 'default_manifest'}, 'service': {'name': 'default_manifest', 'requires': ['docker'], 'after': ['docker'], 'before': [], 'dependent-of': [], 'asic-service': False, 'host-service': False, 'warm-shutdown': {'after': [], 'before': []}, 'fast-shutdown': + #{'after': [], 'before': []}, 'syslog': {'support-rate-limit': False}}, 'container': {'privileged': False, 'volumes': [], 'tmpfs': [], 'entrypoint': ''}, 'cli': { 'mandatory': False, 'config': [], 'show': [], 'clear': []}} +MANIFEST_LOCATION = "/var/lib/sonic-package-manager/manifests/" +DEFAUT_MANIFEST_NAME = "default_manifest" +DMFILE_NAME = os.path.join(MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME) class ManifestSchema: """ ManifestSchema class describes and provides marshalling @@ -212,6 +279,7 @@ def unmarshal(self, value): ]), ManifestRoot('container', [ ManifestField('privileged', DefaultMarshaller(bool), False), + ManifestField('entrypoint', DefaultMarshaller(str), ''), ManifestArray('volumes', DefaultMarshaller(str)), ManifestArray('mounts', ManifestRoot('mounts', [ ManifestField('source', DefaultMarshaller(str)), diff --git a/sonic_package_manager/metadata.py b/sonic_package_manager/metadata.py index b44b658a74..9c5dc43abb 100644 --- a/sonic_package_manager/metadata.py +++ b/sonic_package_manager/metadata.py @@ -4,14 +4,14 @@ import json import tarfile -from typing import Dict, List - +from typing import Dict, List, Optional +import os from sonic_package_manager import utils from sonic_package_manager.errors import MetadataError from sonic_package_manager.logger import log from sonic_package_manager.manifest import Manifest from sonic_package_manager.version import Version - +from sonic_package_manager.manifest import DEFAULT_MANIFEST, MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME, DMFILE_NAME def translate_plain_to_tree(plain: Dict[str, str], sep='.') -> Dict: """ Convert plain key/value dictionary into @@ -65,7 +65,47 @@ def __init__(self, docker, registry_resolver): self.docker = docker self.registry_resolver = registry_resolver - def from_local(self, image: str) -> Metadata: + def get_manifest_from_local_file(self, name, custom_name: Optional[str] = None): + if name is None: + return None + + if not os.path.exists(MANIFEST_LOCATION): + os.mkdir(MANIFEST_LOCATION) + if name == DEFAUT_MANIFEST_NAME and not os.path.exists(DMFILE_NAME): + with open(DMFILE_NAME, 'w') as file: + json.dump(DEFAULT_MANIFEST, file, indent=4) + #print(f"Manifest '{DEFAUT_MANIFEST_NAME}' created now") + + manifest_path = os.path.join(MANIFEST_LOCATION, name) + if os.path.exists(manifest_path): + with open(manifest_path, 'r') as file: + manifest_dict = json.load(file) + if custom_name: + manifest_dict["package"]["name"] = custom_name + manifest_dict["service"]["name"] = custom_name + + #create a manifest file with custom_name, if name is default_manifest + new_manifest_path = os.path.join(MANIFEST_LOCATION, custom_name) + with open(new_manifest_path, 'w') as file: + json.dump(manifest_dict, file, indent=4) + + json_str = json.dumps(manifest_dict, indent=4) + desired_dict = { + 'Tag': 'master', + 'com': { + 'azure': { + 'sonic': { + 'manifest': json_str + } + } + } + } + return desired_dict + else: + print("Local Manifest file {} does not exists".format(manifest_path)) + return None + + def from_local(self, image: str, use_local_manifest: bool = False, name: Optional[str] = None, use_edit: bool = False) -> Metadata: """ Reads manifest from locally installed docker image. Args: @@ -75,16 +115,31 @@ def from_local(self, image: str) -> Metadata: Raises: MetadataError """ + if name and (use_local_manifest or use_edit): + edit_file_name = name + '.edit' + if use_edit: + labels = self.get_manifest_from_local_file(edit_file_name) + return self.from_labels(labels) + elif use_local_manifest: + labels = self.get_manifest_from_local_file(name) + return self.from_labels(labels) labels = self.docker.labels(image) - if labels is None: - raise MetadataError('No manifest found in image labels') + if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: + if name: + labels = self.get_manifest_from_local_file(name) + if labels is None: + raise MetadataError('No manifest found in image labels and also could not create locally') + else: + raise MetadataError('No manifest found in image labels and also could not create locally') return self.from_labels(labels) def from_registry(self, repository: str, - reference: str) -> Metadata: + reference: str, + use_local_manifest: bool = False, + name: Optional[str] = None) -> Metadata: """ Reads manifest from remote registry. Args: @@ -96,19 +151,26 @@ def from_registry(self, MetadataError """ - registry = self.registry_resolver.get_registry_for(repository) + if use_local_manifest: + labels = self.get_manifest_from_local_file(name) + return self.from_labels(labels) + registry = self.registry_resolver.get_registry_for(repository) manifest = registry.manifest(repository, reference) digest = manifest['config']['digest'] blob = registry.blobs(repository, digest) labels = blob['config']['Labels'] + if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: + if name is None: + raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') + custom_name = name + labels = self.get_manifest_from_local_file("default_manifest", custom_name) if labels is None: - raise MetadataError('No manifest found in image labels') - + raise MetadataError('No manifest found in image labels and also could not create locally') return self.from_labels(labels) - def from_tarball(self, image_path: str) -> Metadata: + def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: Optional[str] = None) -> Metadata: """ Reads manifest image tarball. Args: image_path: Path to image tarball. @@ -117,6 +179,9 @@ def from_tarball(self, image_path: str) -> Metadata: Raises: MetadataError """ + if use_local_manifest: + labels = self.get_manifest_from_local_file(name) + return self.from_labels(labels) with tarfile.open(image_path) as image: manifest = json.loads(image.extractfile('manifest.json').read()) @@ -124,9 +189,13 @@ def from_tarball(self, image_path: str) -> Metadata: blob = manifest[0]['Config'] image_config = json.loads(image.extractfile(blob).read()) labels = image_config['config']['Labels'] - if labels is None: - raise MetadataError('No manifest found in image labels') - + if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: + if name is None: + raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') + custom_name = name + labels = self.get_manifest_from_local_file("default_manifest", custom_name) + if labels is None: + raise MetadataError('No manifest found in image labels and also could not create locally') return self.from_labels(labels) @classmethod @@ -176,5 +245,5 @@ def from_labels(cls, labels: Dict[str, str]) -> Metadata: log.debug(f"Found YANG modules: {labels_yang_modules.keys()}") else: log.debug("No YANG modules found") - + return Metadata(Manifest.marshal(manifest_dict), components, yang_modules) diff --git a/sonic_package_manager/service_creator/creator.py b/sonic_package_manager/service_creator/creator.py index 15d3aedd76..5bce07bbea 100644 --- a/sonic_package_manager/service_creator/creator.py +++ b/sonic_package_manager/service_creator/creator.py @@ -235,6 +235,11 @@ def generate_container_mgmt(self, package: Package): if container_spec['privileged']: run_opt.append('--privileged') + entrypoint_arg = '' + ep = container_spec['entrypoint'] + if ep: + entrypoint_arg = ep + run_opt.append('-t') for volume in container_spec['volumes']: @@ -255,6 +260,7 @@ def generate_container_mgmt(self, package: Package): 'docker_container_name': name, 'docker_image_id': image_id, 'docker_image_run_opt': run_opt, + 'docker_entrypoint': entrypoint_arg } render_template(script_template, script_path, render_ctx, executable=True) log.info(f'generated {script_path}') diff --git a/sonic_package_manager/source.py b/sonic_package_manager/source.py index 7a13dccbac..5a6d8ff179 100644 --- a/sonic_package_manager/source.py +++ b/sonic_package_manager/source.py @@ -4,7 +4,7 @@ from sonic_package_manager.dockerapi import DockerApi, get_repository_from_image from sonic_package_manager.metadata import Metadata, MetadataResolver from sonic_package_manager.package import Package - +from typing import Optional class PackageSource(object): """ PackageSource abstracts the way manifest is read @@ -105,20 +105,22 @@ def __init__(self, tarball_path: str, database: PackageDatabase, docker: DockerApi, - metadata_resolver: MetadataResolver): + metadata_resolver: MetadataResolver, + use_local_manifest: bool = False, + name: Optional[str] = None): super().__init__(database, docker, metadata_resolver) self.tarball_path = tarball_path + self.use_local_manifest = use_local_manifest + self.name = name def get_metadata(self) -> Metadata: """ Returns manifest read from tarball. """ - - return self.metadata_resolver.from_tarball(self.tarball_path) + return self.metadata_resolver.from_tarball(self.tarball_path, use_local_manifest=self.use_local_manifest, name=self.name) def install_image(self, package: Package): """ Installs image from local tarball source. """ - return self.docker.load(self.tarball_path) @@ -131,18 +133,24 @@ def __init__(self, reference: str, database: PackageDatabase, docker: DockerApi, - metadata_resolver: MetadataResolver): + metadata_resolver: MetadataResolver, + use_local_manifest: bool = False, + name: Optional[str] = None): super().__init__(database, docker, metadata_resolver) self.repository = repository self.reference = reference + self.use_local_manifest = use_local_manifest + self.name = name def get_metadata(self) -> Metadata: """ Returns manifest read from registry. """ return self.metadata_resolver.from_registry(self.repository, - self.reference) + self.reference, + self.use_local_manifest, + self.name) def install_image(self, package: Package): """ Installs image from registry. """ @@ -161,11 +169,17 @@ def __init__(self, entry: PackageEntry, database: PackageDatabase, docker: DockerApi, - metadata_resolver: MetadataResolver): + metadata_resolver: MetadataResolver, + use_local_manifest: bool = False, + name: Optional[str] = None, + use_edit: bool = False): super().__init__(database, docker, metadata_resolver) self.entry = entry + self.use_local_manifest = use_local_manifest + self.name = name + self.use_edit = use_edit def get_metadata(self) -> Metadata: """ Returns manifest read from locally installed Docker. """ @@ -177,8 +191,7 @@ def get_metadata(self) -> Metadata: # won't have image_id in database. Using their # repository name as image. image = f'{self.entry.repository}:latest' - - return self.metadata_resolver.from_local(image) + return self.metadata_resolver.from_local(image, self.use_local_manifest, self.name, self.use_edit) def get_package(self) -> Package: return Package(self.entry, self.get_metadata()) diff --git a/tests/sonic_package_manager/conftest.py b/tests/sonic_package_manager/conftest.py index 10fe72cac1..ccfc2f4929 100644 --- a/tests/sonic_package_manager/conftest.py +++ b/tests/sonic_package_manager/conftest.py @@ -133,20 +133,20 @@ def __init__(self): self.add('Azure/docker-test-6', '2.0.0', 'test-package-6', '2.0.0') self.add('Azure/docker-test-6', 'latest', 'test-package-6', '1.5.0') - def from_registry(self, repository: str, reference: str): + def from_registry(self, repository: str, reference: str, use_local_manifest=None, name=None): manifest = Manifest.marshal(self.metadata_store[repository][reference]['manifest']) components = self.metadata_store[repository][reference]['components'] yang = self.metadata_store[repository][reference]['yang'] return Metadata(manifest, components, yang) - def from_local(self, image: str): + def from_local(self, image: str, use_local_manfiest=None, name=None, use_edit=None): ref = Reference.parse(image) manifest = Manifest.marshal(self.metadata_store[ref['name']][ref['tag']]['manifest']) components = self.metadata_store[ref['name']][ref['tag']]['components'] yang = self.metadata_store[ref['name']][ref['tag']]['yang'] return Metadata(manifest, components, yang) - def from_tarball(self, filepath: str) -> Manifest: + def from_tarball(self, filepath: str, use_local_manifest=None, name=None) -> Manifest: path, ref = filepath.split(':') manifest = Manifest.marshal(self.metadata_store[path][ref]['manifest']) components = self.metadata_store[path][ref]['components'] diff --git a/tests/sonic_package_manager/test_cli.py b/tests/sonic_package_manager/test_cli.py index 695d8cba58..385108efb5 100644 --- a/tests/sonic_package_manager/test_cli.py +++ b/tests/sonic_package_manager/test_cli.py @@ -4,6 +4,16 @@ from sonic_package_manager import main +from unittest.mock import patch, Mock, mock_open, MagicMock +import os + +MANIFEST_LOCATION = 'fake_manifest_location' +DMFILE_NAME = 'fake_dmfile_name' +DEFAUT_MANIFEST_NAME = 'fake_default_manifest_name' +LOCAL_JSON = 'fake_local_json' +sample_manifest_json = '{"package": {"name": "test", "version": "1.0.0"}, "service": {"name": "test"}}' +fake_manifest_name = 'test-manifest' +MANIFEST_CONTENT = '{"package": {"name": "test", "version": "1.0.0"}, "service": {"name": "test"}}' def test_show_changelog(package_manager, fake_metadata_resolver): """ Test case for "sonic-package-manager package show changelog [NAME]" """ @@ -61,3 +71,587 @@ def test_show_changelog_no_changelog(package_manager): assert result.exit_code == 1 assert result.output == 'Failed to print package changelog: No changelog for package test-package\n' + + +def test_manifests_create_command(package_manager): + + runner = CliRunner() + + with patch('os.path.exists', return_value=False), \ + patch('os.mkdir'), \ + patch('os.path.isfile', return_value=False), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('builtins.open', new_callable=mock_open()) as mock_file, \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) + + assert 'Manifest \'test-manifest\' created successfully.' in result.output + +def test_manifests_create_command_existing_manifest(package_manager): + """ Test case for "sonic-package-manager manifests create" with an existing manifest file """ + + runner = CliRunner() + + with patch('os.path.exists', side_effect=[True, False]), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('builtins.open', new_callable=mock_open()) as mock_file, \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) + + assert 'Error: Manifest file \'test-manifest\' already exists.' in result.output + assert result.exit_code == 0 + +def test_manifests_create_command_existing_package(package_manager): + """ Test case for "sonic-package-manager manifests create" with an existing installed package """ + + runner = CliRunner() + + with patch('os.path.exists', return_value=False), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('builtins.open', new_callable=mock_open()) as mock_file, \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) + + assert 'Error: A package with the same name test-manifest is already installed' in result.output + assert result.exit_code == 0 + + + +def test_manifests_update_command(package_manager): + + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ + patch('json.dump'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + assert 'Manifest \'test-manifest\' updated successfully.' in result.output + + +def test_manifests_update_command_error_handling(package_manager): + + runner = CliRunner() + + with patch('os.path.exists', return_value=False), \ + patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ + patch('json.dump'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['non-existent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + assert 'Local Manifest file for non-existent-manifest does not exists to update' in result.output + assert result.exit_code == 0 + +def test_manifests_update_command_download_failure(package_manager): + """ Test case for "sonic-package-manager manifests update" with download failure """ + + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ + patch('sonic_package_manager.main.download_file', side_effect=Exception('Download failed')), \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ + patch('shutil.copy'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + assert "Error occurred while updating manifest 'test-manifest': Download failed" in result.output + + +def test_manifests_update_nonexistent_file_command(package_manager): + + runner = CliRunner() + + # Test case: Updating a non-existent manifest file + with patch('os.path.exists', return_value=False), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ + patch('json.dump'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['test-nonexistent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + assert f"Local Manifest file for test-nonexistent-manifest does not exists to update" in result.output + + +def test_manifests_update_command_installed(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ + patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file') as mock_download_file, \ + patch('sonic_package_manager.main.Manifest.marshal'), \ + patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ + patch('json.dump'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + assert 'Manifest \'test-manifest\' updated successfully.' in result.output + + # Assuming 'test-manifest' is the name and the package is already installed + edit_name = 'test-manifest.edit' + EDIT_FILE = os.path.join('/path/to/manifests', edit_name) + + # Check if the .edit file is created + assert os.path.exists(EDIT_FILE) + + +def test_manifests_update_command_error(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ + patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ + patch('shutil.copy'), \ + patch('sonic_package_manager.main.download_file', side_effect=Exception('Download failed')), \ + patch('sonic_package_manager.main.Manifest.marshal', side_effect=Exception('Validation failed')), \ + patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ + patch('json.dump'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + # Check if the error message is present in the result output + assert 'Error occurred while updating manifest \'test-manifest\': Download failed' in result.output + + +def test_manifests_delete_command_not_found(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=False), \ + patch('click.prompt', return_value='n'), \ + patch('os.remove') as mock_remove, \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) + + # Check if the error message is present in the result output + assert 'Error: Manifest file \'test-manifest\' not found.' in result.output + # Check if os.remove is not called when the manifest file is not found + assert not mock_remove.called + +def test_manifests_delete_command_deletion_cancelled(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('click.prompt', return_value='n'), \ + patch('os.remove') as mock_remove, \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) + + # Check if the cancellation message is present in the result output + assert 'Deletion cancelled.' in result.output + # Check if os.remove is not called when the deletion is cancelled + assert not mock_remove.called + + +def test_manifests_delete_command_failed_deletion(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('click.prompt', return_value='y'), \ + patch('os.remove', side_effect=Exception('Deletion failed')), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) + + # Check if the failed deletion error message is present in the result output + assert 'Error: Failed to delete manifest file \'test-manifest\'. Deletion failed' in result.output + + +def test_manifests_delete_command(package_manager): + + runner = CliRunner() + + with patch('os.path.exists', return_value=True), \ + patch('os.remove'), \ + patch('os.path.join', return_value='fake_manifest_file_path'), \ + patch('click.prompt', return_value='y'), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['delete'], [fake_manifest_name], obj=package_manager) + + assert 'Manifest \'test-manifest\' deleted successfully.' in result.output + + +def test_manifests_list_command(package_manager): + + runner = CliRunner() + + with patch('os.listdir', return_value=['test-manifest1', 'test-manifest2']), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['list'], obj=package_manager) + + assert 'Custom Local Manifest files:' in result.output + assert '- test-manifest1' in result.output + assert '- test-manifest2' in result.output + +def test_manifests_list_command_no_manifests(package_manager): + runner = CliRunner() + + with patch('os.listdir', return_value=[]), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['list'], [], obj=package_manager) + + # Check if the appropriate message is present in the result output + assert 'No custom local manifest files found.' in result.output + + +def test_manifests_show_command(package_manager): + + runner = CliRunner() + + with patch('builtins.open', mock_open(read_data=MANIFEST_CONTENT)), \ + patch('os.path.exists', return_value=True), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['show'], ['test-manifest'], obj=package_manager) + + assert 'Manifest file: test-manifest' in result.output + assert '"package": {' in result.output + assert '"name": "test"' in result.output + assert '"version": "1.0.0"' in result.output + assert '"service": {' in result.output + assert '"name": "test"' in result.output + +def test_manifests_show_command_file_not_found(package_manager): + runner = CliRunner() + + with patch('os.path.exists', return_value=False), \ + patch('os.geteuid', return_value=0): + + result = runner.invoke(main.manifests.commands['show'], ['nonexistent-manifest'], obj=package_manager) + + # Check if the appropriate error message is present in the result output + assert 'Manifest file \'nonexistent-manifest\' not found.' in result.output + + +def test_download_file(): + runner = CliRunner() + + with runner.isolated_filesystem(): + # Set up a fake remote file and local path + fake_remote_path = "remote_path" + fake_local_path = "local_path" + + # Mock parse_url to return SCP information + with patch("sonic_package_manager.main.parse_url", return_value=("test_user", None, "test_host", fake_remote_path)): + # Mock SSHClient and SFTP + with patch("paramiko.SSHClient") as mock_ssh_client, patch("paramiko.SFTP") as mock_sftp: + # Mock getpass to return a test password + with patch("getpass.getpass", return_value="test_password"): + # Mock os.path.exists to simulate the remote file existence + with patch("os.path.exists", return_value=True): + # Mock open to simulate writing to a local file + with patch("builtins.open", mock_open()) as mock_file: + main.download_file(f"scp://{fake_remote_path}", fake_local_path) + + # Assertions + mock_ssh_client.assert_called_once() + mock_ssh_client().set_missing_host_key_policy.assert_called_once() + mock_ssh_client().connect.assert_called_once_with("test_host", username="test_user", password="test_password") + mock_ssh_client().close.assert_called_once() + + # Additional assertions based on your specific logic... + + # Assertions for HTTP scenario + with runner.isolated_filesystem(): + fake_remote_url = "http://example.com/remote_file" + fake_local_path = "local_path" + + with patch("sonic_package_manager.main.parse_url", return_value=(None, None, "example.com", "/remote_file")): + with patch("requests.get") as mock_requests_get: + with patch("builtins.open", mock_open()) as mock_file: + main.download_file(fake_remote_url, fake_local_path) + + mock_requests_get.assert_called_once_with(fake_remote_url) + mock_file.assert_called_once_with("local_path", "wb") + + # Assertions for local file scenario + with runner.isolated_filesystem(): + fake_local_file = "local_file" + fake_local_path = "local_path" + + with patch("sonic_package_manager.main.parse_url", return_value=(None, None, None, "/local_file")): + with patch("os.path.exists", return_value=True): + with patch("shutil.copy") as mock_shutil_copy: + main.download_file(fake_local_file, fake_local_path) + + mock_shutil_copy.assert_called_once_with("/local_file", "local_path") + + +def test_download_file_error(capsys): + runner = CliRunner() + + # Mock requests.get to simulate an exception during HTTP download + with runner.isolated_filesystem(): + fake_remote_url = "http://example.com/remote_file" + fake_local_path = "local_path" + + with patch("sonic_package_manager.main.parse_url", return_value=(None, None, "example.com", "/remote_file")): + with patch("requests.get", side_effect=requests.exceptions.RequestException("Download failed")) as mock_requests_get: + with patch("builtins.open", mock_open()) as mock_file: + main.download_file(fake_remote_url, fake_local_path) + + mock_requests_get.assert_called_once_with(fake_remote_url) + mock_file.assert_not_called() # Ensure that file is not opened in case of exception + #assert "Download error Download failed" in capsys.readouterr().err # Adjust this line based on your output logic + out, err = capsys.readouterr() + assert "Image file not found on remote machine. Aborting..." in out + assert "Download error Download failed" in out + + +def test_manifests_command(): + """ Test case for "sonic-package-manager manifests" """ + + runner = CliRunner() + result = runner.invoke(main.manifests) + assert result.exit_code == 0 + + +def test_show_package_manifest(package_manager): + """ Test case for sonic-package-manager show package manifest """ + + runner = CliRunner() + result = runner.invoke(main.show.commands['package'].commands['manifest'], ['test-package'], obj=package_manager) + + print("result:===={}".format(result.output)) + assert result.exit_code == 0 + + +def test_show_manifest_error_handling(package_manager): + """ Test case for error handling for sonic-package-manager show package manifest """ + + runner = CliRunner() + result = runner.invoke(main.show.commands['package'].commands['manifest'], ['invalid-package'], obj=package_manager) + + print("result:===={}".format(result.output)) + assert result.exit_code != 0 + assert 'Failed to print manifest' in result.output + +from sonic_package_manager.main import parse_url +def test_download_file_with_scp_url(): + url = "scp://user:pass@hostname:/path/to/file" + + with patch('sonic_package_manager.main.parse_url', return_value=('user', 'pass', 'hostname', '/path/to/file')): + # Call the function that uses parse_url + username, password, hostname, remote_path = parse_url(url) + + # Now, you can assert the values or perform other checks + assert username == 'user' + assert password == 'pass' + assert hostname == 'hostname' + assert remote_path == '/path/to/file' + + +def test_download_file_with_http_url(): + url = "http://example.com/path/to/file" + + with patch('sonic_package_manager.main.parse_url', return_value=(None, None, 'example.com', '/path/to/file')): + # Call the function that uses parse_url + username, password, hostname, remote_path = parse_url(url) + + # Now, you can assert the values or perform other checks + assert username is None + assert password is None + assert hostname == 'example.com' + assert remote_path == '/path/to/file' + +def test_download_file_with_local_path(): + url = "/path/to/local/file" + + with patch('sonic_package_manager.main.parse_url', return_value=(None, None, None, '/path/to/local/file')): + # Call the function that uses parse_url + username, password, hostname, remote_path = parse_url(url) + + # Now, you can assert the values or perform other checks + assert username is None + assert password is None + assert hostname is None + assert remote_path == '/path/to/local/file' + + +from urllib.parse import urlparse + +def test_parse_url_unsupported_scheme(): + url = "ftp://example.com/some/file" + + with patch('sonic_package_manager.main.urlparse', return_value=urlparse(url)): + try: + # Call the function that uses parse_url with an unsupported scheme + parse_url(url) + except ValueError as e: + # Assert that the ValueError is raised with the expected message + assert str(e) == "Unsupported URL scheme" + else: + # If no exception is raised, fail the test + assert False, "Expected ValueError but no exception was raised" + + +from sonic_package_manager.main import validate_url_or_abort +import requests + +def test_validate_url_or_abort_successful(): + url = "http://example.com/some/file" + + with patch('sonic_package_manager.main.requests.head') as mock_head: + # Simulate a successful HTTP HEAD request + mock_head.return_value.status_code = 200 + + # Call the function with the URL + validate_url_or_abort(url) + + # Assert that the print statements are not called + mock_head.assert_called_once_with(url) + +def test_validate_url_or_abort_request_exception(): + url = "http://example.com/some/file" + + with patch('sonic_package_manager.main.requests.head') as mock_head, \ + patch('builtins.print') as mock_print: + # Simulate a requests exception + mock_head.side_effect = requests.exceptions.RequestException() + + # Call the function with the URL + validate_url_or_abort(url) + + # Assert that the appropriate print statement is called + mock_head.assert_called_once_with(url) + mock_print.assert_called_once_with("Did not receive a response from remote machine. Aborting...") + +def test_validate_url_or_abort_4xx_response(): + url = "http://example.com/some/file" + + with patch('sonic_package_manager.main.requests.head') as mock_head, \ + patch('builtins.print') as mock_print: + # Simulate a 4xx response code + mock_head.return_value.status_code = 404 + + # Call the function with the URL + validate_url_or_abort(url) + + # Assert that the appropriate print statement is called + mock_head.assert_called_once_with(url) + mock_print.assert_called_once_with("Image file not found on remote machine. Aborting...") + + + +def test_install_with_local_manifest(package_manager, monkeypatch): + runner = CliRunner() + + with runner.isolated_filesystem(): + # Create a fake local manifest file and set use_local_manifest to True + fake_name = "fake_manifest" + fake_repo = "fake_repo" + fake_local_path = "local_path" + fake_local_manifest_content = '{"package": {"name": "test_package"}}' + + with open(fake_local_path, 'w') as fake_manifest_file: + fake_manifest_file.write(fake_local_manifest_content) + + monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) + + with patch('os.geteuid', return_value=0): + # Run the install command with use_local_manifest and name arguments + result = runner.invoke(main.install, ['--from-repository', fake_repo, '--use-local-manifest', '--name', fake_name], obj=package_manager) + + print("result:{}".format(result.output)) + # Assertions + assert result.exit_code == 0 + assert f'Local Manifest file for {fake_name} does not exists to install' in result.output + #assert f'name argument is not provided to use local manifest' in result.output + + with patch('os.geteuid', return_value=0): + ## Run the install command with use_local_manifest, name, and non-existent local manifest + result = runner.invoke(main.install, ['--from-repository', fake_name, '--use-local-manifest']) + + print("result:{}".format(result.output)) + ## Assertions + assert result.exit_code == 0 + assert f'name argument is not provided to use local manifest' in result.output + + + + +def test_update(package_manager, monkeypatch): + runner = CliRunner() + + with runner.isolated_filesystem(): + fake_package_name = "fake_package" + fake_force = True + fake_yes = False + + # Mock PackageManager.update method + with patch.object(package_manager, 'update') as mock_update: + # Use monkeypatch to bypass the click.confirm prompt + monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) + + with patch('os.geteuid', return_value=0): + # Run the update command with arguments + result = runner.invoke(main.update, [fake_package_name, '--force', '--yes'], obj=package_manager) + + print("result:{}".format(result.output)) + ## Assertions + mock_update.assert_called_once_with(fake_package_name, fake_force) + assert result.exit_code == 0 + + +class SomeUpdateError(Exception): + pass + +def test_update_error(package_manager, monkeypatch): + runner = CliRunner() + + with runner.isolated_filesystem(): + fake_package_name = "fake_package" + fake_force = True + fake_yes = False + + # Mock PackageManager.update method to raise an exception + with patch.object(package_manager, 'update', side_effect=SomeUpdateError("Update failed")) as mock_update: + # Use monkeypatch to bypass the click.confirm prompt + monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) + + with patch('os.geteuid', return_value=0): + # Run the update command with arguments + result = runner.invoke(main.update, [fake_package_name, '--force', '--yes'], obj=package_manager) + + # Assertions + mock_update.assert_called_once_with(fake_package_name, fake_force) + assert result.exit_code == 1 + assert "Failed to update package fake_package: Update failed" in result.output + + + diff --git a/tests/sonic_package_manager/test_manager.py b/tests/sonic_package_manager/test_manager.py index 46ea3f6acb..9a74b0b295 100644 --- a/tests/sonic_package_manager/test_manager.py +++ b/tests/sonic_package_manager/test_manager.py @@ -4,6 +4,7 @@ from unittest.mock import Mock, call, patch import pytest +import mock import sonic_package_manager from sonic_package_manager.errors import * @@ -389,3 +390,31 @@ def test_manager_migration_dockerd(package_manager, fake_db_for_migration, mock_ package_manager.migrate_packages(fake_db_for_migration, '/var/run/docker.sock') package_manager.get_docker_client.assert_has_calls([ call('/var/run/docker.sock')], any_order=True) + + +def test_manager_update(package_manager, mock_docker_api, mock_service_creator, fake_metadata_resolver, anything): + # Install the initial version + package_manager.install('test-package=1.6.0') + + # Mock the metadata for the updated version + updated_manifest = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest'] + updated_components = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['components'] + + # Update the metadata resolver to return the updated version metadata + fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest'] = updated_manifest + fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['components'] = updated_components + + # Mock service creator methods + mock_service_creator.create.side_effect = lambda pkg, **opts: print(f'Creating service for {pkg.name}') + mock_service_creator.remove.side_effect = lambda pkg, **opts: print(f'Removing service for {pkg.name}') + mock_service_creator.generate_shutdown_sequence_files.side_effect = lambda pkgs: print(f'Generating shutdown sequence files for {pkgs}') + + edit_path = '/var/lib/sonic-package-manager/manifests/test-package.edit' + manifest_path = '/var/lib/sonic-package-manager/manifests/test-package' + + with mock.patch('os.rename', side_effect=lambda src, dest: None): + package_manager.update('test-package') + + # Assertions for service creator calls + mock_service_creator.remove.assert_called_once_with(anything, deregister_feature=False) + diff --git a/tests/sonic_package_manager/test_metadata.py b/tests/sonic_package_manager/test_metadata.py index 96f9bbc38d..bcc59d00ad 100644 --- a/tests/sonic_package_manager/test_metadata.py +++ b/tests/sonic_package_manager/test_metadata.py @@ -2,14 +2,13 @@ import json import contextlib -from unittest.mock import Mock, MagicMock - +from unittest.mock import Mock, MagicMock, patch, mock_open import pytest from sonic_package_manager.database import PackageEntry from sonic_package_manager.errors import MetadataError from sonic_package_manager.manifest import Manifest -from sonic_package_manager.metadata import MetadataResolver +from sonic_package_manager.metadata import MetadataResolver, Metadata from sonic_package_manager.version import Version @@ -87,3 +86,70 @@ def test_metadata_construction(manifest_str): }) assert metadata.yang_modules == ['TEST', 'TEST 2'] + +@patch("sonic_package_manager.metadata.os.path.exists", return_value=True) +@patch("sonic_package_manager.metadata.open", create=True) +def test_get_manifest_from_local_file(mock_open, mock_path_exists, mock_docker_api, mock_registry_resolver): + # Setting up mock file content with required fields + mock_file_content = { + "package": { + "name": "test-package", + "version": "1.0.0", + }, + "service": { + "name": "test-package", + "asic-service": False, + "host-service": True, + }, + "container": { + "privileged": True, + }, + } + mock_open.return_value.__enter__.return_value.read.return_value = json.dumps(mock_file_content) + + # Creating a mock MetadataResolver + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + + # Mocking necessary dependencies in the MetadataResolver instance + metadata_resolver.docker.labels = Mock(return_value={"com.azure.sonic.manifest": "mocked_manifest_labels"}) + + # Testing with an existing local manifest + metadata = metadata_resolver.from_local('test-image', use_local_manifest=True, name='test-package', use_edit=False) + + assert isinstance(metadata, Metadata) + + + +def test_get_manifest_from_local_file2(capsys): + metadata_resolver = MetadataResolver(None, None) # Replace None with appropriate mocks + + with patch('os.path.exists', return_value=True), \ + patch('os.mkdir'), \ + patch('builtins.open', mock_open(read_data=json.dumps({"package": {"name": "test-package"}}))): + # Test when manifest file exists + manifest = metadata_resolver.get_manifest_from_local_file('test-package') + assert manifest is not None + + with patch('os.path.exists', return_value=False), \ + patch('os.mkdir'), \ + patch('builtins.open', mock_open()): + # Test when manifest file does not exist + manifest = metadata_resolver.get_manifest_from_local_file('non-existent-package') + captured = capsys.readouterr() + assert captured.out.strip() == "Local Manifest file /var/lib/sonic-package-manager/manifests/non-existent-package does not exists" + assert manifest is None + + manifest_data = {"package": {"name": "test-package"}, "service": {"name": "test-package"}} + with patch('os.path.exists', return_value=True), \ + patch('os.mkdir'), \ + patch('builtins.open', mock_open(read_data=json.dumps(manifest_data))), \ + patch('json.load', return_value=manifest_data), \ + patch('json.dump') as mock_json_dump: + # Test when custom_name is provided + manifest = metadata_resolver.get_manifest_from_local_file('test-package', custom_name='custom-name') + + captured = capsys.readouterr() + print("Captured Output:", captured.out) + + # Assert that the manifest is not None + assert manifest is not None diff --git a/tests/sonic_package_manager/test_service_creator.py b/tests/sonic_package_manager/test_service_creator.py index 8e6edcd0f0..d0d2050d42 100644 --- a/tests/sonic_package_manager/test_service_creator.py +++ b/tests/sonic_package_manager/test_service_creator.py @@ -43,6 +43,7 @@ def manifest(): }, 'container': { 'privileged': True, + 'entrypoint': '', 'volumes': [ '/etc/sonic:/etc/sonic:ro' ] @@ -602,3 +603,39 @@ def test_rate_limit_update(self, mock_sonic_db, manifest): ], any_order = True ) + + +def test_generate_container_mgmt_entrypoint(): + # Mock Package object with necessary attributes + package_mock = Mock() + package_mock.image_id = "some_image_id" + package_mock.manifest = { + 'service': {'name': 'test'}, + 'container': { + 'privileged': True, + 'entrypoint': '/custom_entrypoint', + 'volumes': [], + 'mounts': [], + 'tmpfs': [], + 'environment': {} + } + } + + mock_feature_registry = Mock() + mock_sonic_db = Mock() + mock_cli_gen = Mock() + mock_config_mgmt = Mock() + + # Mock constants + with patch('sonic_package_manager.service_creator.creator.DOCKER_CTL_SCRIPT_LOCATION', '/usr/bin/'), \ + patch('sonic_package_manager.service_creator.creator.DOCKER_CTL_SCRIPT_TEMPLATE', 'docker_ctl_script_template.tmpl'), \ + patch('sonic_package_manager.service_creator.creator.get_tmpl_path', return_value='mocked_template_path'), \ + patch('sonic_package_manager.service_creator.creator.render_template') as mock_render_template, \ + patch('sonic_package_manager.service_creator.creator.log.info') as mock_log_info: + + srv_instance = ServiceCreator(mock_feature_registry, mock_sonic_db, mock_cli_gen, mock_config_mgmt) + srv_instance.generate_container_mgmt(package_mock) + + # Assertions + mock_render_template.assert_called_once() + mock_log_info.assert_called_once_with('generated /usr/bin/test.sh') From f5b85dd322600011fb7adc89303ab4e4a90d0e2e Mon Sep 17 00:00:00 2001 From: sg893052 Date: Thu, 21 Mar 2024 06:33:56 -0700 Subject: [PATCH 2/4] Thrid Party Container Management using Sonic Package Manager - followup1 --- setup.py | 1 + sonic_installer/main.py | 4 + sonic_package_manager/main.py | 228 +------- sonic_package_manager/manager.py | 387 +++++++++---- sonic_package_manager/manifest.py | 52 +- sonic_package_manager/metadata.py | 73 +-- .../service_creator/creator.py | 6 - tests/sonic_package_manager/test_cli.py | 516 +++--------------- tests/sonic_package_manager/test_manager.py | 267 ++++++++- tests/sonic_package_manager/test_manifest.py | 21 +- tests/sonic_package_manager/test_metadata.py | 146 +++-- tests/test_sonic_installer.py | 2 + 12 files changed, 794 insertions(+), 909 deletions(-) diff --git a/setup.py b/setup.py index 196777d0e3..a989acb876 100644 --- a/setup.py +++ b/setup.py @@ -257,6 +257,7 @@ 'xmltodict==0.12.0', 'lazy-object-proxy', 'six==1.16.0', + 'scp==0.14.5', ] + sonic_dependencies, setup_requires= [ 'pytest-runner', diff --git a/sonic_installer/main.py b/sonic_installer/main.py index 341111f265..b099d6d119 100644 --- a/sonic_installer/main.py +++ b/sonic_installer/main.py @@ -337,6 +337,8 @@ def migrate_sonic_packages(bootloader, binary_image_version): new_image_docker_mount = os.path.join(new_image_mount, "var", "lib", "docker") docker_default_config = os.path.join(new_image_mount, "etc", "default", "docker") docker_default_config_backup = os.path.join(new_image_mount, TMP_DIR, "docker_config_backup") + custom_manifests_path = os.path.join(PACKAGE_MANAGER_DIR, "manifests") + new_image_package_directory_path = os.path.join(new_image_mount, "var", "lib", "sonic-package-manager") if not os.path.isdir(new_image_docker_dir): # NOTE: This codepath can be reached if the installation process did not @@ -372,6 +374,8 @@ def migrate_sonic_packages(bootloader, binary_image_version): run_command_or_raise(["chroot", new_image_mount, DOCKER_CTL_SCRIPT, "start"]) docker_started = True run_command_or_raise(["cp", packages_path, os.path.join(new_image_mount, TMP_DIR, packages_file)]) + run_command_or_raise(["mkdir", "-p", custom_manifests_path]) + run_command_or_raise(["cp", "-arf", custom_manifests_path , new_image_package_directory_path]) run_command_or_raise(["touch", os.path.join(new_image_mount, "tmp", DOCKERD_SOCK)]) run_command_or_raise(["mount", "--bind", os.path.join(VAR_RUN_PATH, DOCKERD_SOCK), diff --git a/sonic_package_manager/main.py b/sonic_package_manager/main.py index 4943a64f34..af3b0ecfb3 100644 --- a/sonic_package_manager/main.py +++ b/sonic_package_manager/main.py @@ -13,16 +13,13 @@ import paramiko import requests import getpass -import shutil from natsort import natsorted from sonic_package_manager.database import PackageEntry, PackageDatabase from sonic_package_manager.errors import PackageManagerError from sonic_package_manager.logger import log from sonic_package_manager.manager import PackageManager -from sonic_package_manager.manifest import Manifest, DEFAULT_MANIFEST, MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME, DMFILE_NAME -LOCAL_TARBALL_PATH="/tmp/local_tarball.gz" -LOCAL_JSON="/tmp/local_json" +from sonic_package_manager.manifest import MANIFESTS_LOCATION BULLET_UC = '\u2022' @@ -105,8 +102,11 @@ def handle_parse_result(self, ctx, opts, args): cls=MutuallyExclusiveOption, mutually_exclusive=['from_tarball', 'package_expr']), click.option('--from-tarball', - type=str, - help='Fetch package from saved image tarball from local/scp/sftp/http', + type=click.Path(exists=True, + readable=True, + file_okay=True, + dir_okay=False), + help='Fetch package from saved image tarball.', cls=MutuallyExclusiveOption, mutually_exclusive=['from_repository', 'package_expr']), click.argument('package-expr', @@ -227,11 +227,6 @@ def manifest(ctx, manager: PackageManager = ctx.obj try: - if from_tarball: - #Download the tar file from local/scp/sftp/http - download_file(from_tarball, LOCAL_TARBALL_PATH) - from_tarball = LOCAL_TARBALL_PATH - source = manager.get_package_source(package_expr, from_repository, from_tarball) @@ -272,11 +267,6 @@ def changelog(ctx, manager: PackageManager = ctx.obj try: - if from_tarball: - #Download the tar file from local/scp/sftp/http - download_file(from_tarball, LOCAL_TARBALL_PATH) - from_tarball = LOCAL_TARBALL_PATH - source = manager.get_package_source(package_expr, from_repository, from_tarball) @@ -311,52 +301,13 @@ def changelog(ctx, def create(ctx, name, from_json): """Create a new custom local manifest file.""" - #Validation checks manager: PackageManager = ctx.obj - if manager.is_installed(name): - click.echo("Error: A package with the same name {} is already installed".format(name)) - return - MFILE_NAME = os.path.join(MANIFEST_LOCATION, name) - if os.path.exists(MFILE_NAME): - click.echo("Error: Manifest file '{}' already exists.".format(name)) - return - - #Creation of default manifest file in case the file does not exist - if not os.path.exists(MANIFEST_LOCATION): - os.mkdir(MANIFEST_LOCATION) - if not os.path.exists(DMFILE_NAME): - with open(DMFILE_NAME, 'w') as file: - json.dump(DEFAULT_MANIFEST, file, indent=4) - #click.echo(f"Manifest '{DEFAUT_MANIFEST_NAME}' created now.") - - - #Create the manifest file in centralized location - #Download the json file from scp/sftp/http to local_json_file - try: - if from_json: - download_file(from_json, LOCAL_JSON) - from_json = LOCAL_JSON - data = {} - with open(from_json, 'r') as file: - data = json.load(file) - #Validate with manifest scheme - Manifest.marshal(data) - - #Make sure the 'name' is overwritten into the dict - data['package']['name'] = name - data['service']['name'] = name - - with open(MFILE_NAME, 'w') as file: - json.dump(data, file, indent=4) - else: - shutil.copy(DMFILE_NAME, MFILE_NAME) - click.echo(f"Manifest '{name}' created successfully.") + try: + manager.create_package_manifest(name, from_json) except Exception as e: click.echo("Error: Manifest {} creation failed - {}".format(name, str(e))) return - - #At the end of sonic-package-manager install, a new manifest file is created with the name. #At the end of sonic-package-manager uninstall name, this manifest file name and name.edit will be deleted. #At the end of sonic-package-manager update, we need to mv maniests name.edit to name in case of success, else keep it as such. @@ -365,67 +316,26 @@ def create(ctx, name, from_json): @click.pass_context @click.argument('name', type=click.Path()) @click.option('--from-json', type=str, required=True) -#@click.argument('--from-json', type=str, help='Specify Manifest json file') @root_privileges_required def update(ctx, name, from_json): """Update an existing custom local manifest file with new one.""" manager: PackageManager = ctx.obj - ORG_FILE = os.path.join(MANIFEST_LOCATION, name) - if not os.path.exists(ORG_FILE): - click.echo(f'Local Manifest file for {name} does not exists to update') - return try: - #download json file from remote/local path - download_file(from_json, LOCAL_JSON) - from_json = LOCAL_JSON - with open(from_json, 'r') as file: - data = json.load(file) - - #Validate with manifest scheme - Manifest.marshal(data) - - #Make sure the 'name' is overwritten into the dict - data['package']['name'] = name - data['service']['name'] = name - - if manager.is_installed(name): - edit_name = name + '.edit' - EDIT_FILE = os.path.join(MANIFEST_LOCATION, edit_name) - with open(EDIT_FILE, 'w') as edit_file: - json.dump(data, edit_file, indent=4) - click.echo(f"Manifest '{name}' updated successfully.") - else: - #If package is not installed, - ## update the name file directly - with open(ORG_FILE, 'w') as orig_file: - json.dump(data, orig_file, indent=4) - click.echo(f"Manifest '{name}' updated successfully.") + manager.update_package_manifest(name, from_json) except Exception as e: click.echo(f"Error occurred while updating manifest '{name}': {e}") return - @manifests.command('delete') @click.pass_context @click.argument('name', type=click.Path()) @root_privileges_required def delete(ctx, name): """Delete a custom local manifest file.""" - # Check if the manifest file exists - mfile_name = "{}{}".format(MANIFEST_LOCATION, name) - if not os.path.exists(mfile_name): - click.echo("Error: Manifest file '{}' not found.".format(name)) - return - + manager: PackageManager = ctx.obj try: - # Confirm deletion with user input - confirm = click.prompt("Are you sure you want to delete the manifest file '{}'? (y/n)".format(name), type=str) - if confirm.lower() == 'y': - os.remove(mfile_name) - click.echo("Manifest '{}' deleted successfully.".format(name)) - else: - click.echo("Deletion cancelled.") + manager.delete_package_manifest(name) except Exception as e: click.echo("Error: Failed to delete manifest file '{}'. {}".format(name, e)) @@ -436,15 +346,9 @@ def delete(ctx, name): @root_privileges_required def show_manifest(ctx, name): """Show the contents of custom local manifest file.""" - mfile_name = "{}{}".format(MANIFEST_LOCATION, name) - edit_file_name = "{}.edit".format(mfile_name) + manager: PackageManager = ctx.obj try: - if os.path.exists(edit_file_name): - mfile_name = edit_file_name - with open(mfile_name, 'r') as file: - data = json.load(file) - click.echo("Manifest file: {}".format(name)) - click.echo(json.dumps(data, indent=4)) + manager.show_package_manifest(name) except FileNotFoundError: click.echo("Manifest file '{}' not found.".format(name)) @@ -453,14 +357,8 @@ def show_manifest(ctx, name): @root_privileges_required def list_manifests(ctx): """List all custom local manifest files.""" - # Get all files in the manifest location - manifest_files = os.listdir(MANIFEST_LOCATION) - if not manifest_files: - click.echo("No custom local manifest files found.") - else: - click.echo("Custom Local Manifest files:") - for file in manifest_files: - click.echo("- {}".format(file)) + manager: PackageManager = ctx.obj + manager.list_package_manifest() @repository.command() @@ -499,78 +397,6 @@ def remove(ctx, name): exit_cli(f'Failed to remove repository {name}: {err}', fg='red') -def parse_url(url): - # Parse information from URL - parsed_url = urlparse(url) - if parsed_url.scheme == "scp" or parsed_url.scheme == "sftp": - return parsed_url.username, parsed_url.password, parsed_url.hostname, parsed_url.path - elif parsed_url.scheme == "http": - return None, None, parsed_url.netloc, parsed_url.path - elif not parsed_url.scheme: # No scheme indicates a local file path - return None, None, None, parsed_url.path - else: - raise ValueError("Unsupported URL scheme") - -def validate_url_or_abort(url): - # Attempt to retrieve HTTP response code - try: - response = requests.head(url) - response_code = response.status_code - except requests.exceptions.RequestException as err: - response_code = None - - if not response_code: - print("Did not receive a response from remote machine. Aborting...") - return - else: - # Check for a 4xx response code which indicates a nonexistent URL - if str(response_code).startswith('4'): - print("Image file not found on remote machine. Aborting...") - return - -def download_file(url, local_path): - # Parse information from the URL - username, password, hostname, remote_path = parse_url(url) - - if username is not None: - # If password is not provided, prompt the user for it securely - if password is None: - password = getpass.getpass(prompt=f"Enter password for {username}@{hostname}: ") - - # Create an SSH client for SCP or SFTP - client = paramiko.SSHClient() - # Automatically add the server's host key (this is insecure and should be handled differently in production) - client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) - - try: - # Connect to the SSH server - client.connect(hostname, username=username, password=password) - - # Open an SCP channel for SCP or an SFTP channel for SFTP - with client.open_sftp() as sftp: - # Download the file - sftp.get(remote_path, local_path) - - finally: - # Close the SSH connection - client.close() - elif hostname: - # Download using HTTP for URLs without credentials - validate_url_or_abort(url) - try: - response = requests.get(url) - with open(local_path, 'wb') as f: - f.write(response.content) - except requests.exceptions.RequestException as e: - print("Download error", e) - return - else: - if os.path.exists(remote_path): - shutil.copy(remote_path, local_path) - else: - print(f"Error: Source file '{remote_path}' does not exist.") - - @cli.command() @click.option('--enable', is_flag=True, @@ -592,7 +418,8 @@ def download_file(url, local_path): @click.option('--use-local-manifest', is_flag=True, default=None, - help='Use locally created custom manifest file ') + help='Use locally created custom manifest file. ', + hidden=True) @click.option('--name', type=str, help='custom name for the package') @@ -646,16 +473,11 @@ def install(ctx, if not name: click.echo(f'name argument is not provided to use local manifest') return - ORG_FILE = os.path.join(MANIFEST_LOCATION, name) - if not os.path.exists(ORG_FILE): + original_file = os.path.join(MANIFESTS_LOCATION, name) + if not os.path.exists(original_file): click.echo(f'Local Manifest file for {name} does not exists to install') return - if from_tarball: - #Download the tar file from local/scp/sftp/http - download_file(from_tarball, LOCAL_TARBALL_PATH) - from_tarball = LOCAL_TARBALL_PATH - try: manager.install(package_expr, from_repository, @@ -671,16 +493,22 @@ def install(ctx, @cli.command() @add_options(PACKAGE_COMMON_OPERATION_OPTIONS) +@add_options(PACKAGE_COMMON_INSTALL_OPTIONS) @click.argument('name') @click.pass_context @root_privileges_required -def update(ctx, name, force, yes): - """ Update package to the updated manifest file """ +def update(ctx, name, force, yes, skip_host_plugins): + """ Update package to the updated manifest file. """ manager: PackageManager = ctx.obj + update_opts = { + 'force': force, + 'skip_host_plugins': skip_host_plugins, + 'update_only': True, + } try: - manager.update(name, force) + manager.update(name, **update_opts) except Exception as err: exit_cli(f'Failed to update package {name}: {err}', fg='red') except KeyboardInterrupt: diff --git a/sonic_package_manager/manager.py b/sonic_package_manager/manager.py index 7832760073..bba93973c3 100644 --- a/sonic_package_manager/manager.py +++ b/sonic_package_manager/manager.py @@ -65,8 +65,16 @@ version_to_tag, tag_to_version ) -from sonic_package_manager.manifest import MANIFEST_LOCATION -LOCAL_TARBALL_PATH="/tmp/local_tarball.gz" +import click +import json +import os +import requests +import getpass +import paramiko +import urllib.parse +from scp import SCPClient +from sonic_package_manager.manifest import Manifest, DEFAULT_MANIFEST, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE +LOCAL_JSON="/tmp/local_json" @contextlib.contextmanager def failure_ignore(ignore: bool): @@ -433,7 +441,7 @@ def install_from_source(self, self.service_creator.generate_shutdown_sequence_files, self.get_installed_packages()) ) - + if not skip_host_plugins: self._install_cli_plugins(package) exits.callback(rollback(self._uninstall_cli_plugins, package)) @@ -450,72 +458,36 @@ def install_from_source(self, self.database.commit() @under_lock - @opt_check - def update(self, name: str, - force: bool = False): + def update(self, + name: str, + **kwargs): """ Update SONiC Package referenced by name. The update can be forced if force argument is True. Args: name: SONiC Package name. - use_local_manifest: Use this manifest to update - force: Force the installation. Raises: PackageManagerError """ + if self.is_installed(name): + edit_name = name + '.edit' + edit_file = os.path.join(MANIFESTS_LOCATION, edit_name) + if os.path.exists(edit_file): + self.upgrade_from_source(None, name=name, **kwargs) + else: + click.echo("Package manifest {}.edit file does not exists to update".format(name)) + return + else: + click.echo("Package {} is not installed".format(name)) + return - with failure_ignore(force): - if not self.is_installed(name): - raise PackageUninstallationError(f'{name} is not installed') - - old_package = self.get_installed_package(name) - new_package = self.get_installed_package(name, use_edit=True) - - service_create_opts = { - 'register_feature': False, - } - service_remove_opts = { - 'deregister_feature': False, - } - try: - with contextlib.ExitStack() as exits: - self.service_creator.remove(old_package, **service_remove_opts) - exits.callback(rollback(self.service_creator.create, old_package, - **service_create_opts)) - - self.docker.rm_by_ancestor(old_package.image_id, force=True) - - self.service_creator.create(new_package, **service_create_opts) - exits.callback(rollback(self.service_creator.remove, new_package, - **service_remove_opts)) - - self.service_creator.generate_shutdown_sequence_files( - self._get_installed_packages_and(new_package) - ) - exits.callback(rollback( - self.service_creator.generate_shutdown_sequence_files, - self._get_installed_packages_and(old_package)) - ) - - self.feature_registry.update(old_package.manifest, new_package.manifest) - exits.callback(rollback( - self.feature_registry.update, new_package.manifest, old_package.manifest) - ) - - exits.pop_all() - except Exception as err: - raise PackageUpgradeError(f'Failed to update {new_package.name}: {err}') - except KeyboardInterrupt: - raise - - new_package_entry = new_package.entry - new_package_entry.installed = True - self.database.update_package(new_package_entry) - self.database.commit() - manifest_path = os.path.join(MANIFEST_LOCATION, name) - edit_path = os.path.join(MANIFEST_LOCATION, name + ".edit") - os.rename(edit_path,manifest_path) + def remove_unused_docker_image(self, package): + image_id_used = any(entry.image_id == package.image_id for entry in self.database if entry.name != package.name) + if not image_id_used: + self.docker.rmi(package.image_id, force=True) + else: + log.info(f'Image with ID {package.image_id} is in use by other package(s). Skipping deletion') @under_lock @opt_check @@ -540,9 +512,6 @@ def uninstall(self, name: str, package = self.get_installed_package(name) service_name = package.manifest['service']['name'] - image_id_used = False - image_id_used = any(entry.image_id == package.image_id for entry in self.database if entry.name != package.name) - with failure_ignore(force): if self.feature_registry.is_feature_enabled(service_name): raise PackageUninstallationError( @@ -568,10 +537,7 @@ def uninstall(self, name: str, ) self.docker.rm_by_ancestor(package.image_id, force=True) # Delete image if it is not in use, otherwise skip deletion - if not image_id_used: - self.docker.rmi(package.image_id, force=True) - else: - print(f'Image with ID {package.image_id} is in use by other package(s). Skipping deletion') + self.remove_unused_docker_image(package) package.entry.image_id = None except Exception as err: raise PackageUninstallationError( @@ -582,8 +548,8 @@ def uninstall(self, name: str, package.entry.version = None self.database.update_package(package.entry) self.database.commit() - manifest_path = os.path.join(MANIFEST_LOCATION, name) - edit_path = os.path.join(MANIFEST_LOCATION, name + ".edit") + manifest_path = os.path.join(MANIFESTS_LOCATION, name) + edit_path = os.path.join(MANIFESTS_LOCATION, name + ".edit") if os.path.exists(manifest_path): os.remove(manifest_path) if os.path.exists(edit_path): @@ -596,7 +562,9 @@ def upgrade_from_source(self, source: PackageSource, force=False, skip_host_plugins=False, - allow_downgrade=False): + allow_downgrade=False, + update_only: Optional[bool] = False, + name: Optional[str] = None): """ Upgrade SONiC Package to a version the package reference expression specifies. Can force the upgrade if force parameter is True. Force can allow a package downgrade. @@ -606,12 +574,17 @@ def upgrade_from_source(self, force: Force the upgrade. skip_host_plugins: Skip host OS plugins installation. allow_downgrade: Flag to allow package downgrade. + update_only: Perform package update with new manifest. + name: name of package. Raises: PackageManagerError """ - - new_package = source.get_package() - name = new_package.name + + if update_only: + new_package = self.get_installed_package(name, use_edit=True) + else: + new_package = source.get_package() + name = new_package.name with failure_ignore(force): if not self.is_installed(name): @@ -628,19 +601,20 @@ def upgrade_from_source(self, old_version = old_package.manifest['package']['version'] new_version = new_package.manifest['package']['version'] - with failure_ignore(force): - if old_version == new_version: - raise PackageUpgradeError(f'{new_version} is already installed') - - # TODO: Not all packages might support downgrade. - # We put a check here but we understand that for some packages - # the downgrade might be safe to do. There can be a variable in manifest - # describing package downgrade ability or downgrade-able versions. - if new_version < old_version and not allow_downgrade: - raise PackageUpgradeError( - f'Request to downgrade from {old_version} to {new_version}. ' - f'Downgrade might be not supported by the package' - ) + if not update_only: + with failure_ignore(force): + if old_version == new_version: + raise PackageUpgradeError(f'{new_version} is already installed') + + # TODO: Not all packages might support downgrade. + # We put a check here but we understand that for some packages + # the downgrade might be safe to do. There can be a variable in manifest + # describing package downgrade ability or downgrade-able versions. + if new_version < old_version and not allow_downgrade: + raise PackageUpgradeError( + f'Request to downgrade from {old_version} to {new_version}. ' + f'Downgrade might be not supported by the package' + ) # remove currently installed package from the list installed_packages = self._get_installed_packages_and(new_package) @@ -664,8 +638,9 @@ def upgrade_from_source(self, self._uninstall_cli_plugins(old_package) exits.callback(rollback(self._install_cli_plugins, old_package)) - source.install(new_package) - exits.callback(rollback(source.uninstall, new_package)) + if not update_only: + source.install(new_package) + exits.callback(rollback(source.uninstall, new_package)) feature_enabled = self.feature_registry.is_feature_enabled(old_feature) @@ -704,13 +679,9 @@ def upgrade_from_source(self, if not skip_host_plugins: self._install_cli_plugins(new_package) exits.callback(rollback(self._uninstall_cli_plugin, new_package)) - - old_image_id_used = False - old_image_id_used = any(entry.image_id == old_package.image_id for entry in self.database if entry.name != old_package.name) - if not old_image_id_used or old_package.image_id != new_package.image_id: - self.docker.rmi(old_package.image_id, force=True) - else: - print(f'Image with ID {old_package.image_id} is in use by other package(s). Skipping deletion') + + if old_package.image_id != new_package.image_id: + self.remove_unused_docker_image(old_package) exits.pop_all() except Exception as err: @@ -723,6 +694,10 @@ def upgrade_from_source(self, new_package_entry.version = new_version self.database.update_package(new_package_entry) self.database.commit() + if update_only: + manifest_path = os.path.join(MANIFESTS_LOCATION, name) + edit_path = os.path.join(MANIFESTS_LOCATION, name + ".edit") + os.rename(edit_path,manifest_path) @under_lock @opt_check @@ -808,7 +783,7 @@ def migrate_package(old_package_entry, file.write(chunk) file.flush() - self.install(tarball=file.name) + self.install(tarball=file.name, name=name) else: log.info(f'installing {name} version {version}') @@ -838,10 +813,10 @@ def migrate_package(old_package_entry, package_source = self.get_package_source(package_ref=new_package_ref) package = package_source.get_package() new_package_default_version = package.manifest['package']['version'] - if old_package.version > new_package_default_version: + if old_package.version >= new_package_default_version: log.info(f'{old_package.name} package version is lower ' - f'then the default in new image: ' - f'{old_package.version} > {new_package_default_version}') + f'then or equal to the default in new image: ' + f'{old_package.version} >= {new_package_default_version}') new_package.version = old_package.version migrate_package(old_package, new_package) else: @@ -895,7 +870,7 @@ def get_package_source(self, if package_expression: ref = parse_reference_expression(package_expression) - return self.get_package_source(package_ref=ref, name=name) + return self.get_package_source(package_ref=ref) elif repository_reference: repo_ref = utils.DockerReference.parse(repository_reference) repository = repo_ref['name'] @@ -1121,6 +1096,220 @@ def _uninstall_cli_plugin(self, package: Package, command: str): if os.path.exists(host_plugin_path): os.remove(host_plugin_path) + def download_file(self, url, local_path): + # Parse information from the URL + parsed_url = urllib.parse.urlparse(url) + protocol = parsed_url.scheme + username = parsed_url.username + password = parsed_url.password + hostname = parsed_url.hostname + remote_path = parsed_url.path + supported_protocols = ['http', 'https', 'scp', 'sftp'] + + #clear the temporary local file + if os.path.exists(local_path): + os.remove(local_path) + + if not protocol: + #check for local file + if os.path.exists(url): + os.rename(url, local_path) + return True + else: + click.echo("Local file not present") + return False + if protocol not in supported_protocols: + click.echo("Protocol not supported") + return False + + # If the protocol is HTTP and no username or password is provided, proceed with the download using requests + if (protocol == 'http' or protocol == 'https') and not username and not password: + try: + with requests.get(url, stream=True) as response: + response.raise_for_status() + with open(local_path, 'wb') as f: + for chunk in response.iter_content(chunk_size=8192): + if chunk: + f.write(chunk) + except requests.exceptions.RequestException as e: + click.echo("Download error", e) + return False + else: + # If password is not provided, prompt the user for it securely + if password is None: + password = getpass.getpass(prompt=f"Enter password for {username}@{hostname}: ") + + # Create an SSH client + client = paramiko.SSHClient() + # Automatically add the server's host key (this is insecure and should be handled differently in production) + client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) + + try: + # Connect to the SSH server + client.connect(hostname, username=username, password=password) + + if protocol == 'scp': + # Create an SCP client + scp = SCPClient(client.get_transport()) + # Download the file + scp.get(remote_path, local_path) + elif protocol == 'sftp': + # Open an SFTP channel + with client.open_sftp() as sftp: + # Download the file + sftp.get(remote_path, local_path) + elif protocol == 'http' or protocol == 'https': + # Download using HTTP for URLs without credentials + try: + with requests.get(url, auth=(username, password), stream=True) as response: + response.raise_for_status() # Raise an exception if the request was not successful + with open(local_path, 'wb') as f: + for chunk in response.iter_content(chunk_size=8192): + if chunk: + f.write(chunk) + except requests.exceptions.RequestException as e: + click.echo("Download error", e) + return False + else: + click.echo(f"Error: Source file '{remote_path}' does not exist.") + + finally: + # Close the SSH connection + client.close() + + def create_package_manifest(self, name, from_json): + if name == "default_manifest": + click.echo("Default Manifest creation is not allowed by user") + return + if self.is_installed(name): + click.echo("Error: A package with the same name {} is already installed".format(name)) + return + mfile_name = os.path.join(MANIFESTS_LOCATION, name) + if os.path.exists(mfile_name): + click.echo("Error: Manifest file '{}' already exists.".format(name)) + return + + #Creation of default manifest file in case the file does not exist + if not os.path.exists(MANIFESTS_LOCATION): + os.mkdir(MANIFESTS_LOCATION) + if not os.path.exists(DEFAULT_MANIFEST_FILE): + with open(DEFAULT_MANIFEST_FILE, 'w') as file: + json.dump(DEFAULT_MANIFEST, file, indent=4) + + if from_json: + ret = self.download_file(from_json, LOCAL_JSON) + if ret is False: + return + from_json = LOCAL_JSON + else: + from_json = DEFAULT_MANIFEST_FILE + data = {} + with open(from_json, 'r') as file: + data = json.load(file) + #Validate with manifest scheme + Manifest.marshal(data) + + #Make sure the 'name' is overwritten into the dict + data['package']['name'] = name + data['service']['name'] = name + + with open(mfile_name, 'w') as file: + json.dump(data, file, indent=4) + click.echo(f"Manifest '{name}' created successfully.") + + def check_manifests_directory_existence(self): + if not os.path.exists(MANIFESTS_LOCATION): + click.echo("Manifests files directory empty") + return False + + def update_package_manifest(self, name, from_json): + if name == "default_manifest": + click.echo("Default Manifest updation is not allowed") + return + + ret = self.check_manifests_directory_existence() + if ret is False: + return + original_file = os.path.join(MANIFESTS_LOCATION, name) + if not os.path.exists(original_file): + click.echo(f'Local Manifest file for {name} does not exists to update') + return + #download json file from remote/local path + ret = self.download_file(from_json, LOCAL_JSON) + if ret is False: + return + from_json = LOCAL_JSON + + with open(from_json, 'r') as file: + data = json.load(file) + + #Validate with manifest scheme + Manifest.marshal(data) + + #Make sure the 'name' is overwritten into the dict + data['package']['name'] = name + data['service']['name'] = name + + if self.is_installed(name): + edit_name = name + '.edit' + edit_file = os.path.join(MANIFESTS_LOCATION, edit_name) + with open(edit_file, 'w') as edit_file: + json.dump(data, edit_file, indent=4) + click.echo(f"Manifest '{name}' updated successfully.") + else: + #If package is not installed, + ## update the name file directly + with open(original_file, 'w') as orig_file: + json.dump(data, orig_file, indent=4) + click.echo(f"Manifest '{name}' updated successfully.") + + def delete_package_manifest(self, name): + if name == "default_manifest": + click.echo("Default Manifest deletion is not allowed") + return + ret = self.check_manifests_directory_existence() + if ret is False: + return + # Check if the manifest file exists + mfile_name = "{}{}".format(MANIFESTS_LOCATION, name) + if not os.path.exists(mfile_name): + click.echo("Error: Manifest file '{}' not found.".format(name)) + return + # Confirm deletion with user input + confirm = click.prompt("Are you sure you want to delete the manifest file '{}'? (y/n)".format(name), type=str) + if confirm.lower() == 'y': + os.remove(mfile_name) + click.echo("Manifest '{}' deleted successfully.".format(name)) + else: + click.echo("Deletion cancelled.") + return + + def show_package_manifest(self, name): + ret = self.check_manifests_directory_existence() + if ret is False: + return + mfile_name = "{}{}".format(MANIFESTS_LOCATION, name) + edit_file_name = "{}.edit".format(mfile_name) + if os.path.exists(edit_file_name): + mfile_name = edit_file_name + with open(mfile_name, 'r') as file: + data = json.load(file) + click.echo("Manifest file: {}".format(name)) + click.echo(json.dumps(data, indent=4)) + + def list_package_manifest(self): + ret = self.check_manifests_directory_existence() + if ret is False: + return + # Get all files in the manifest location + manifest_files = os.listdir(MANIFESTS_LOCATION) + if not manifest_files: + click.echo("No custom local manifest files found.") + else: + click.echo("Custom Local Manifest files:") + for file in manifest_files: + click.echo("- {}".format(file)) + @staticmethod def get_manager() -> 'PackageManager': """ Creates and returns PackageManager instance. diff --git a/sonic_package_manager/manifest.py b/sonic_package_manager/manifest.py index e87fb50776..0c00cb9093 100644 --- a/sonic_package_manager/manifest.py +++ b/sonic_package_manager/manifest.py @@ -12,6 +12,7 @@ from sonic_package_manager.version import Version import os +import json DEFAULT_MANIFEST = { "version": "1.0.0", @@ -73,11 +74,9 @@ "auto-generate-config-source-yang-modules": [] } } -#DEFAULT_MANIFEST = {'version': '1.0.0', 'package': {'version': '1.0.0', 'depends': [], 'name': 'default_manifest'}, 'service': {'name': 'default_manifest', 'requires': ['docker'], 'after': ['docker'], 'before': [], 'dependent-of': [], 'asic-service': False, 'host-service': False, 'warm-shutdown': {'after': [], 'before': []}, 'fast-shutdown': - #{'after': [], 'before': []}, 'syslog': {'support-rate-limit': False}}, 'container': {'privileged': False, 'volumes': [], 'tmpfs': [], 'entrypoint': ''}, 'cli': { 'mandatory': False, 'config': [], 'show': [], 'clear': []}} -MANIFEST_LOCATION = "/var/lib/sonic-package-manager/manifests/" -DEFAUT_MANIFEST_NAME = "default_manifest" -DMFILE_NAME = os.path.join(MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME) +MANIFESTS_LOCATION = "/var/lib/sonic-package-manager/manifests/" +DEFAULT_MANIFEST_NAME = "default_manifest" +DEFAULT_MANIFEST_FILE = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) class ManifestSchema: """ ManifestSchema class describes and provides marshalling @@ -279,7 +278,6 @@ def unmarshal(self, value): ]), ManifestRoot('container', [ ManifestField('privileged', DefaultMarshaller(bool), False), - ManifestField('entrypoint', DefaultMarshaller(str), ''), ManifestArray('volumes', DefaultMarshaller(str)), ManifestArray('mounts', ManifestRoot('mounts', [ ManifestField('source', DefaultMarshaller(str)), @@ -317,3 +315,45 @@ def marshal(cls, input_dict: dict): def unmarshal(self) -> Dict: return self.SCHEMA.unmarshal(self) + + def get_manifest_from_local_file(name): + + if not os.path.exists(MANIFESTS_LOCATION): + os.mkdir(MANIFESTS_LOCATION) + if not os.path.exists(DEFAULT_MANIFEST_FILE): + with open(DEFAULT_MANIFEST_FILE, 'w') as file: + json.dump(DEFAULT_MANIFEST, file, indent=4) + + if '.edit' in name: + actual_name = name.split('.edit')[0] + else: + actual_name = name + + manifest_path = os.path.join(MANIFESTS_LOCATION, name) + if os.path.exists(manifest_path): + with open(manifest_path, 'r') as file: + manifest_dict = json.load(file) + manifest_dict["package"]["name"] = actual_name + manifest_dict["service"]["name"] = actual_name + else: + with open(DEFAULT_MANIFEST_FILE, 'r') as file: + manifest_dict = json.load(file) + manifest_dict["package"]["name"] = actual_name + manifest_dict["service"]["name"] = actual_name + new_manifest_path = os.path.join(MANIFESTS_LOCATION, name) + with open(new_manifest_path, 'w') as file: + json.dump(manifest_dict, file, indent=4) + + json_str = json.dumps(manifest_dict, indent=4) + desired_dict = { + 'Tag': 'master', + 'com': { + 'azure': { + 'sonic': { + 'manifest': json_str + } + } + } + } + return desired_dict + diff --git a/sonic_package_manager/metadata.py b/sonic_package_manager/metadata.py index 9c5dc43abb..873f20b82d 100644 --- a/sonic_package_manager/metadata.py +++ b/sonic_package_manager/metadata.py @@ -11,7 +11,6 @@ from sonic_package_manager.logger import log from sonic_package_manager.manifest import Manifest from sonic_package_manager.version import Version -from sonic_package_manager.manifest import DEFAULT_MANIFEST, MANIFEST_LOCATION, DEFAUT_MANIFEST_NAME, DMFILE_NAME def translate_plain_to_tree(plain: Dict[str, str], sep='.') -> Dict: """ Convert plain key/value dictionary into @@ -65,46 +64,6 @@ def __init__(self, docker, registry_resolver): self.docker = docker self.registry_resolver = registry_resolver - def get_manifest_from_local_file(self, name, custom_name: Optional[str] = None): - if name is None: - return None - - if not os.path.exists(MANIFEST_LOCATION): - os.mkdir(MANIFEST_LOCATION) - if name == DEFAUT_MANIFEST_NAME and not os.path.exists(DMFILE_NAME): - with open(DMFILE_NAME, 'w') as file: - json.dump(DEFAULT_MANIFEST, file, indent=4) - #print(f"Manifest '{DEFAUT_MANIFEST_NAME}' created now") - - manifest_path = os.path.join(MANIFEST_LOCATION, name) - if os.path.exists(manifest_path): - with open(manifest_path, 'r') as file: - manifest_dict = json.load(file) - if custom_name: - manifest_dict["package"]["name"] = custom_name - manifest_dict["service"]["name"] = custom_name - - #create a manifest file with custom_name, if name is default_manifest - new_manifest_path = os.path.join(MANIFEST_LOCATION, custom_name) - with open(new_manifest_path, 'w') as file: - json.dump(manifest_dict, file, indent=4) - - json_str = json.dumps(manifest_dict, indent=4) - desired_dict = { - 'Tag': 'master', - 'com': { - 'azure': { - 'sonic': { - 'manifest': json_str - } - } - } - } - return desired_dict - else: - print("Local Manifest file {} does not exists".format(manifest_path)) - return None - def from_local(self, image: str, use_local_manifest: bool = False, name: Optional[str] = None, use_edit: bool = False) -> Metadata: """ Reads manifest from locally installed docker image. @@ -118,20 +77,20 @@ def from_local(self, image: str, use_local_manifest: bool = False, name: Optiona if name and (use_local_manifest or use_edit): edit_file_name = name + '.edit' if use_edit: - labels = self.get_manifest_from_local_file(edit_file_name) + labels = Manifest.get_manifest_from_local_file(edit_file_name) return self.from_labels(labels) elif use_local_manifest: - labels = self.get_manifest_from_local_file(name) + labels = Manifest.get_manifest_from_local_file(name) return self.from_labels(labels) labels = self.docker.labels(image) if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: if name: - labels = self.get_manifest_from_local_file(name) + labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels and also could not create locally') + raise MetadataError('No manifest found in image labels') else: - raise MetadataError('No manifest found in image labels and also could not create locally') + raise MetadataError('No manifest found in image labels') return self.from_labels(labels) @@ -152,7 +111,7 @@ def from_registry(self, """ if use_local_manifest: - labels = self.get_manifest_from_local_file(name) + labels = Manifest.get_manifest_from_local_file(name) return self.from_labels(labels) registry = self.registry_resolver.get_registry_for(repository) @@ -160,14 +119,13 @@ def from_registry(self, digest = manifest['config']['digest'] blob = registry.blobs(repository, digest) - labels = blob['config']['Labels'] + labels = blob['config'].get('Labels') if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: - if name is None: - raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') - custom_name = name - labels = self.get_manifest_from_local_file("default_manifest", custom_name) + if name is None: + raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') + labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels and also could not create locally') + raise MetadataError('No manifest found in image labels') return self.from_labels(labels) def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: Optional[str] = None) -> Metadata: @@ -180,7 +138,7 @@ def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: MetadataError """ if use_local_manifest: - labels = self.get_manifest_from_local_file(name) + labels = Manifest.get_manifest_from_local_file(name) return self.from_labels(labels) with tarfile.open(image_path) as image: @@ -188,14 +146,13 @@ def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: blob = manifest[0]['Config'] image_config = json.loads(image.extractfile(blob).read()) - labels = image_config['config']['Labels'] + labels = image_config['config'].get('Labels') if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: if name is None: raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') - custom_name = name - labels = self.get_manifest_from_local_file("default_manifest", custom_name) + labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels and also could not create locally') + raise MetadataError('No manifest found in image labels') return self.from_labels(labels) @classmethod diff --git a/sonic_package_manager/service_creator/creator.py b/sonic_package_manager/service_creator/creator.py index 5bce07bbea..15d3aedd76 100644 --- a/sonic_package_manager/service_creator/creator.py +++ b/sonic_package_manager/service_creator/creator.py @@ -235,11 +235,6 @@ def generate_container_mgmt(self, package: Package): if container_spec['privileged']: run_opt.append('--privileged') - entrypoint_arg = '' - ep = container_spec['entrypoint'] - if ep: - entrypoint_arg = ep - run_opt.append('-t') for volume in container_spec['volumes']: @@ -260,7 +255,6 @@ def generate_container_mgmt(self, package: Package): 'docker_container_name': name, 'docker_image_id': image_id, 'docker_image_run_opt': run_opt, - 'docker_entrypoint': entrypoint_arg } render_template(script_template, script_path, render_ctx, executable=True) log.info(f'generated {script_path}') diff --git a/tests/sonic_package_manager/test_cli.py b/tests/sonic_package_manager/test_cli.py index 385108efb5..f93632a0c0 100644 --- a/tests/sonic_package_manager/test_cli.py +++ b/tests/sonic_package_manager/test_cli.py @@ -72,24 +72,6 @@ def test_show_changelog_no_changelog(package_manager): assert result.exit_code == 1 assert result.output == 'Failed to print package changelog: No changelog for package test-package\n' - -def test_manifests_create_command(package_manager): - - runner = CliRunner() - - with patch('os.path.exists', return_value=False), \ - patch('os.mkdir'), \ - patch('os.path.isfile', return_value=False), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ - patch('builtins.open', new_callable=mock_open()) as mock_file, \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) - - assert 'Manifest \'test-manifest\' created successfully.' in result.output - def test_manifests_create_command_existing_manifest(package_manager): """ Test case for "sonic-package-manager manifests create" with an existing manifest file """ @@ -97,9 +79,6 @@ def test_manifests_create_command_existing_manifest(package_manager): with patch('os.path.exists', side_effect=[True, False]), \ patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ patch('builtins.open', new_callable=mock_open()) as mock_file, \ patch('os.geteuid', return_value=0): @@ -115,9 +94,6 @@ def test_manifests_create_command_existing_package(package_manager): with patch('os.path.exists', return_value=False), \ patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ patch('builtins.open', new_callable=mock_open()) as mock_file, \ patch('os.geteuid', return_value=0): @@ -126,139 +102,20 @@ def test_manifests_create_command_existing_package(package_manager): assert 'Error: A package with the same name test-manifest is already installed' in result.output assert result.exit_code == 0 - - -def test_manifests_update_command(package_manager): - - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ - patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ - patch('json.dump'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - - assert 'Manifest \'test-manifest\' updated successfully.' in result.output - - def test_manifests_update_command_error_handling(package_manager): runner = CliRunner() with patch('os.path.exists', return_value=False), \ patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ patch('json.dump'), \ patch('os.geteuid', return_value=0): result = runner.invoke(main.manifests.commands['update'], ['non-existent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - assert 'Local Manifest file for non-existent-manifest does not exists to update' in result.output + assert 'Manifests files directory empty\n' in result.output assert result.exit_code == 0 -def test_manifests_update_command_download_failure(package_manager): - """ Test case for "sonic-package-manager manifests update" with download failure """ - - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ - patch('sonic_package_manager.main.download_file', side_effect=Exception('Download failed')), \ - patch('sonic_package_manager.main.Manifest.marshal'), \ - patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ - patch('shutil.copy'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - - assert "Error occurred while updating manifest 'test-manifest': Download failed" in result.output - - -def test_manifests_update_nonexistent_file_command(package_manager): - - runner = CliRunner() - - # Test case: Updating a non-existent manifest file - with patch('os.path.exists', return_value=False), \ - patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ - patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ - patch('json.dump'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['test-nonexistent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - - assert f"Local Manifest file for test-nonexistent-manifest does not exists to update" in result.output - - -def test_manifests_update_command_installed(package_manager): - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ - patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file') as mock_download_file, \ - patch('sonic_package_manager.main.Manifest.marshal'), \ - patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ - patch('json.dump'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - - assert 'Manifest \'test-manifest\' updated successfully.' in result.output - - # Assuming 'test-manifest' is the name and the package is already installed - edit_name = 'test-manifest.edit' - EDIT_FILE = os.path.join('/path/to/manifests', edit_name) - - # Check if the .edit file is created - assert os.path.exists(EDIT_FILE) - - -def test_manifests_update_command_error(package_manager): - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ - patch('builtins.open', new_callable=mock_open(read_data='{"key": "value"}')), \ - patch('shutil.copy'), \ - patch('sonic_package_manager.main.download_file', side_effect=Exception('Download failed')), \ - patch('sonic_package_manager.main.Manifest.marshal', side_effect=Exception('Validation failed')), \ - patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ - patch('json.dump'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - - # Check if the error message is present in the result output - assert 'Error occurred while updating manifest \'test-manifest\': Download failed' in result.output - - -def test_manifests_delete_command_not_found(package_manager): - runner = CliRunner() - - with patch('os.path.exists', return_value=False), \ - patch('click.prompt', return_value='n'), \ - patch('os.remove') as mock_remove, \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) - - # Check if the error message is present in the result output - assert 'Error: Manifest file \'test-manifest\' not found.' in result.output - # Check if os.remove is not called when the manifest file is not found - assert not mock_remove.called - def test_manifests_delete_command_deletion_cancelled(package_manager): runner = CliRunner() @@ -274,49 +131,6 @@ def test_manifests_delete_command_deletion_cancelled(package_manager): # Check if os.remove is not called when the deletion is cancelled assert not mock_remove.called - -def test_manifests_delete_command_failed_deletion(package_manager): - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('click.prompt', return_value='y'), \ - patch('os.remove', side_effect=Exception('Deletion failed')), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) - - # Check if the failed deletion error message is present in the result output - assert 'Error: Failed to delete manifest file \'test-manifest\'. Deletion failed' in result.output - - -def test_manifests_delete_command(package_manager): - - runner = CliRunner() - - with patch('os.path.exists', return_value=True), \ - patch('os.remove'), \ - patch('os.path.join', return_value='fake_manifest_file_path'), \ - patch('click.prompt', return_value='y'), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['delete'], [fake_manifest_name], obj=package_manager) - - assert 'Manifest \'test-manifest\' deleted successfully.' in result.output - - -def test_manifests_list_command(package_manager): - - runner = CliRunner() - - with patch('os.listdir', return_value=['test-manifest1', 'test-manifest2']), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['list'], obj=package_manager) - - assert 'Custom Local Manifest files:' in result.output - assert '- test-manifest1' in result.output - assert '- test-manifest2' in result.output - def test_manifests_list_command_no_manifests(package_manager): runner = CliRunner() @@ -326,25 +140,7 @@ def test_manifests_list_command_no_manifests(package_manager): result = runner.invoke(main.manifests.commands['list'], [], obj=package_manager) # Check if the appropriate message is present in the result output - assert 'No custom local manifest files found.' in result.output - - -def test_manifests_show_command(package_manager): - - runner = CliRunner() - - with patch('builtins.open', mock_open(read_data=MANIFEST_CONTENT)), \ - patch('os.path.exists', return_value=True), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['show'], ['test-manifest'], obj=package_manager) - - assert 'Manifest file: test-manifest' in result.output - assert '"package": {' in result.output - assert '"name": "test"' in result.output - assert '"version": "1.0.0"' in result.output - assert '"service": {' in result.output - assert '"name": "test"' in result.output + assert 'Manifests files directory empty\n' in result.output def test_manifests_show_command_file_not_found(package_manager): runner = CliRunner() @@ -355,83 +151,7 @@ def test_manifests_show_command_file_not_found(package_manager): result = runner.invoke(main.manifests.commands['show'], ['nonexistent-manifest'], obj=package_manager) # Check if the appropriate error message is present in the result output - assert 'Manifest file \'nonexistent-manifest\' not found.' in result.output - - -def test_download_file(): - runner = CliRunner() - - with runner.isolated_filesystem(): - # Set up a fake remote file and local path - fake_remote_path = "remote_path" - fake_local_path = "local_path" - - # Mock parse_url to return SCP information - with patch("sonic_package_manager.main.parse_url", return_value=("test_user", None, "test_host", fake_remote_path)): - # Mock SSHClient and SFTP - with patch("paramiko.SSHClient") as mock_ssh_client, patch("paramiko.SFTP") as mock_sftp: - # Mock getpass to return a test password - with patch("getpass.getpass", return_value="test_password"): - # Mock os.path.exists to simulate the remote file existence - with patch("os.path.exists", return_value=True): - # Mock open to simulate writing to a local file - with patch("builtins.open", mock_open()) as mock_file: - main.download_file(f"scp://{fake_remote_path}", fake_local_path) - - # Assertions - mock_ssh_client.assert_called_once() - mock_ssh_client().set_missing_host_key_policy.assert_called_once() - mock_ssh_client().connect.assert_called_once_with("test_host", username="test_user", password="test_password") - mock_ssh_client().close.assert_called_once() - - # Additional assertions based on your specific logic... - - # Assertions for HTTP scenario - with runner.isolated_filesystem(): - fake_remote_url = "http://example.com/remote_file" - fake_local_path = "local_path" - - with patch("sonic_package_manager.main.parse_url", return_value=(None, None, "example.com", "/remote_file")): - with patch("requests.get") as mock_requests_get: - with patch("builtins.open", mock_open()) as mock_file: - main.download_file(fake_remote_url, fake_local_path) - - mock_requests_get.assert_called_once_with(fake_remote_url) - mock_file.assert_called_once_with("local_path", "wb") - - # Assertions for local file scenario - with runner.isolated_filesystem(): - fake_local_file = "local_file" - fake_local_path = "local_path" - - with patch("sonic_package_manager.main.parse_url", return_value=(None, None, None, "/local_file")): - with patch("os.path.exists", return_value=True): - with patch("shutil.copy") as mock_shutil_copy: - main.download_file(fake_local_file, fake_local_path) - - mock_shutil_copy.assert_called_once_with("/local_file", "local_path") - - -def test_download_file_error(capsys): - runner = CliRunner() - - # Mock requests.get to simulate an exception during HTTP download - with runner.isolated_filesystem(): - fake_remote_url = "http://example.com/remote_file" - fake_local_path = "local_path" - - with patch("sonic_package_manager.main.parse_url", return_value=(None, None, "example.com", "/remote_file")): - with patch("requests.get", side_effect=requests.exceptions.RequestException("Download failed")) as mock_requests_get: - with patch("builtins.open", mock_open()) as mock_file: - main.download_file(fake_remote_url, fake_local_path) - - mock_requests_get.assert_called_once_with(fake_remote_url) - mock_file.assert_not_called() # Ensure that file is not opened in case of exception - #assert "Download error Download failed" in capsys.readouterr().err # Adjust this line based on your output logic - out, err = capsys.readouterr() - assert "Image file not found on remote machine. Aborting..." in out - assert "Download error Download failed" in out - + assert 'Manifests files directory empty\n' in result.output def test_manifests_command(): """ Test case for "sonic-package-manager manifests" """ @@ -441,217 +161,103 @@ def test_manifests_command(): assert result.exit_code == 0 -def test_show_package_manifest(package_manager): - """ Test case for sonic-package-manager show package manifest """ - - runner = CliRunner() - result = runner.invoke(main.show.commands['package'].commands['manifest'], ['test-package'], obj=package_manager) - - print("result:===={}".format(result.output)) - assert result.exit_code == 0 - - -def test_show_manifest_error_handling(package_manager): - """ Test case for error handling for sonic-package-manager show package manifest """ +def test_manifests_create_command_exception(package_manager): + """Test case for "sonic-package-manager manifests create" with an exception during manifest creation""" runner = CliRunner() - result = runner.invoke(main.show.commands['package'].commands['manifest'], ['invalid-package'], obj=package_manager) - - print("result:===={}".format(result.output)) - assert result.exit_code != 0 - assert 'Failed to print manifest' in result.output - -from sonic_package_manager.main import parse_url -def test_download_file_with_scp_url(): - url = "scp://user:pass@hostname:/path/to/file" - - with patch('sonic_package_manager.main.parse_url', return_value=('user', 'pass', 'hostname', '/path/to/file')): - # Call the function that uses parse_url - username, password, hostname, remote_path = parse_url(url) - - # Now, you can assert the values or perform other checks - assert username == 'user' - assert password == 'pass' - assert hostname == 'hostname' - assert remote_path == '/path/to/file' - - -def test_download_file_with_http_url(): - url = "http://example.com/path/to/file" - - with patch('sonic_package_manager.main.parse_url', return_value=(None, None, 'example.com', '/path/to/file')): - # Call the function that uses parse_url - username, password, hostname, remote_path = parse_url(url) - - # Now, you can assert the values or perform other checks - assert username is None - assert password is None - assert hostname == 'example.com' - assert remote_path == '/path/to/file' - -def test_download_file_with_local_path(): - url = "/path/to/local/file" - - with patch('sonic_package_manager.main.parse_url', return_value=(None, None, None, '/path/to/local/file')): - # Call the function that uses parse_url - username, password, hostname, remote_path = parse_url(url) - # Now, you can assert the values or perform other checks - assert username is None - assert password is None - assert hostname is None - assert remote_path == '/path/to/local/file' - - -from urllib.parse import urlparse + with patch('sonic_package_manager.main.PackageManager.create_package_manifest', side_effect=Exception("Custom error")), \ + patch('os.geteuid', return_value=0): -def test_parse_url_unsupported_scheme(): - url = "ftp://example.com/some/file" + result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], obj=package_manager) - with patch('sonic_package_manager.main.urlparse', return_value=urlparse(url)): - try: - # Call the function that uses parse_url with an unsupported scheme - parse_url(url) - except ValueError as e: - # Assert that the ValueError is raised with the expected message - assert str(e) == "Unsupported URL scheme" - else: - # If no exception is raised, fail the test - assert False, "Expected ValueError but no exception was raised" + assert 'Error: Manifest test-manifest creation failed - Custom error' in result.output + assert result.exit_code == 0 +def test_manifests_update_command_exception(package_manager): + """Test case for 'sonic-package-manager manifests update' with an exception during manifest update""" -from sonic_package_manager.main import validate_url_or_abort -import requests + runner = CliRunner() -def test_validate_url_or_abort_successful(): - url = "http://example.com/some/file" + with patch('sonic_package_manager.main.PackageManager.update_package_manifest', side_effect=Exception("Custom error")), \ + patch('os.geteuid', return_value=0): - with patch('sonic_package_manager.main.requests.head') as mock_head: - # Simulate a successful HTTP HEAD request - mock_head.return_value.status_code = 200 + result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'new_manifest.json'], obj=package_manager) - # Call the function with the URL - validate_url_or_abort(url) + assert 'Error occurred while updating manifest \'test-manifest\': Custom error' in result.output + assert result.exit_code == 0 - # Assert that the print statements are not called - mock_head.assert_called_once_with(url) +def test_manifests_delete_command_exception(package_manager): + """Test case for 'sonic-package-manager manifests delete' with an exception during manifest deletion""" -def test_validate_url_or_abort_request_exception(): - url = "http://example.com/some/file" + runner = CliRunner() - with patch('sonic_package_manager.main.requests.head') as mock_head, \ - patch('builtins.print') as mock_print: - # Simulate a requests exception - mock_head.side_effect = requests.exceptions.RequestException() + with patch('sonic_package_manager.main.PackageManager.delete_package_manifest', side_effect=Exception("Custom error")), \ + patch('os.geteuid', return_value=0): - # Call the function with the URL - validate_url_or_abort(url) + result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) - # Assert that the appropriate print statement is called - mock_head.assert_called_once_with(url) - mock_print.assert_called_once_with("Did not receive a response from remote machine. Aborting...") + assert "Error: Failed to delete manifest file 'test-manifest'. Custom error" in result.output + assert result.exit_code == 0 -def test_validate_url_or_abort_4xx_response(): - url = "http://example.com/some/file" +def test_manifests_show_command_file_not_found(package_manager): + """Test case for 'sonic-package-manager manifests show' with a non-existent manifest file""" - with patch('sonic_package_manager.main.requests.head') as mock_head, \ - patch('builtins.print') as mock_print: - # Simulate a 4xx response code - mock_head.return_value.status_code = 404 + runner = CliRunner() - # Call the function with the URL - validate_url_or_abort(url) + with patch('sonic_package_manager.main.PackageManager.show_package_manifest', side_effect=FileNotFoundError()), \ + patch('os.geteuid', return_value=0): - # Assert that the appropriate print statement is called - mock_head.assert_called_once_with(url) - mock_print.assert_called_once_with("Image file not found on remote machine. Aborting...") + result = runner.invoke(main.manifests.commands['show'], ['nonexistent_manifest.json'], obj=package_manager) + assert "Manifest file 'nonexistent_manifest.json' not found." in result.output + assert result.exit_code == 0 +def test_install_with_local_manifest(package_manager): + """Test case for 'install' command with use_local_manifest=True and name provided""" -def test_install_with_local_manifest(package_manager, monkeypatch): runner = CliRunner() - with runner.isolated_filesystem(): - # Create a fake local manifest file and set use_local_manifest to True - fake_name = "fake_manifest" - fake_repo = "fake_repo" - fake_local_path = "local_path" - fake_local_manifest_content = '{"package": {"name": "test_package"}}' - - with open(fake_local_path, 'w') as fake_manifest_file: - fake_manifest_file.write(fake_local_manifest_content) - - monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) - - with patch('os.geteuid', return_value=0): - # Run the install command with use_local_manifest and name arguments - result = runner.invoke(main.install, ['--from-repository', fake_repo, '--use-local-manifest', '--name', fake_name], obj=package_manager) + with patch('os.path.exists', return_value=True), \ + patch('os.geteuid', return_value=0): + result = runner.invoke(main.install, ['package_name', '--use-local-manifest', '-y'], obj=package_manager) - print("result:{}".format(result.output)) - # Assertions - assert result.exit_code == 0 - assert f'Local Manifest file for {fake_name} does not exists to install' in result.output - #assert f'name argument is not provided to use local manifest' in result.output + assert 'name argument is not provided to use local manifest' in result.output + assert result.exit_code == 0 - with patch('os.geteuid', return_value=0): - ## Run the install command with use_local_manifest, name, and non-existent local manifest - result = runner.invoke(main.install, ['--from-repository', fake_name, '--use-local-manifest']) +def test_install_with_nonexistent_manifest(package_manager): + """Test case for 'install' command with use_local_manifest=True and non-existent name provided""" - print("result:{}".format(result.output)) - ## Assertions - assert result.exit_code == 0 - assert f'name argument is not provided to use local manifest' in result.output + runner = CliRunner() + with patch('os.path.exists', return_value=False), \ + patch('os.geteuid', return_value=0): + result = runner.invoke(main.install, ['package_name', '--use-local-manifest', '--name', 'nonexistent_manifest', '-y'], obj=package_manager) + assert 'Local Manifest file for nonexistent_manifest does not exists to install' in result.output + assert result.exit_code == 0 +def test_update_command_exception(package_manager): + """Test case for 'update' command with an exception during package update""" -def test_update(package_manager, monkeypatch): runner = CliRunner() - with runner.isolated_filesystem(): - fake_package_name = "fake_package" - fake_force = True - fake_yes = False - - # Mock PackageManager.update method - with patch.object(package_manager, 'update') as mock_update: - # Use monkeypatch to bypass the click.confirm prompt - monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) + with patch('sonic_package_manager.main.PackageManager.update', side_effect=Exception("Custom error")), \ + patch('os.geteuid', return_value=0): - with patch('os.geteuid', return_value=0): - # Run the update command with arguments - result = runner.invoke(main.update, [fake_package_name, '--force', '--yes'], obj=package_manager) + result = runner.invoke(main.update, ['package_name'], obj=package_manager) - print("result:{}".format(result.output)) - ## Assertions - mock_update.assert_called_once_with(fake_package_name, fake_force) - assert result.exit_code == 0 + assert 'Failed to update package package_name: Custom error' in result.output -class SomeUpdateError(Exception): - pass +def test_update_command_keyboard_interrupt(package_manager): + """Test case for 'update' command with a keyboard interrupt""" -def test_update_error(package_manager, monkeypatch): runner = CliRunner() - with runner.isolated_filesystem(): - fake_package_name = "fake_package" - fake_force = True - fake_yes = False - - # Mock PackageManager.update method to raise an exception - with patch.object(package_manager, 'update', side_effect=SomeUpdateError("Update failed")) as mock_update: - # Use monkeypatch to bypass the click.confirm prompt - monkeypatch.setattr('click.confirm', lambda *args, **kwargs: True) - - with patch('os.geteuid', return_value=0): - # Run the update command with arguments - result = runner.invoke(main.update, [fake_package_name, '--force', '--yes'], obj=package_manager) - - # Assertions - mock_update.assert_called_once_with(fake_package_name, fake_force) - assert result.exit_code == 1 - assert "Failed to update package fake_package: Update failed" in result.output - + with patch('sonic_package_manager.main.PackageManager.update', side_effect=KeyboardInterrupt()), \ + patch('os.geteuid', return_value=0): + result = runner.invoke(main.update, ['package_name'], obj=package_manager) + assert 'Operation canceled by user' in result.output diff --git a/tests/sonic_package_manager/test_manager.py b/tests/sonic_package_manager/test_manager.py index 9a74b0b295..a8fbce77c4 100644 --- a/tests/sonic_package_manager/test_manager.py +++ b/tests/sonic_package_manager/test_manager.py @@ -1,14 +1,15 @@ #!/usr/bin/env python import re -from unittest.mock import Mock, call, patch - +import unittest +from unittest.mock import Mock, call, patch, mock_open, MagicMock import pytest -import mock import sonic_package_manager from sonic_package_manager.errors import * from sonic_package_manager.version import Version +from sonic_package_manager.manifest import Manifest +import json @pytest.fixture(autouse=True) def mock_run_command(): @@ -384,37 +385,247 @@ def save(self, named): return DockerClient(dockerd_sock) -def test_manager_migration_dockerd(package_manager, fake_db_for_migration, mock_docker_api): - package_manager.install = Mock() - package_manager.get_docker_client = Mock(side_effect=mock_get_docker_client) - package_manager.migrate_packages(fake_db_for_migration, '/var/run/docker.sock') - package_manager.get_docker_client.assert_has_calls([ - call('/var/run/docker.sock')], any_order=True) +#def test_manager_migration_dockerd(package_manager, fake_db_for_migration, mock_docker_api): + #package_manager.install = Mock() + #package_manager.get_docker_client = Mock(side_effect=mock_get_docker_client) + #package_manager.migrate_packages(fake_db_for_migration, '/var/run/docker.sock') + #package_manager.get_docker_client.assert_has_calls([ + #call('/var/run/docker.sock')], any_order=True) + + +def test_create_package_manifest_default_manifest(package_manager): + """Test case for creating a default manifest.""" + with patch('os.path.exists', return_value=False), \ + patch('os.mkdir'), \ + patch('builtins.open', new_callable=mock_open()), \ + patch('click.echo') as mock_echo: -def test_manager_update(package_manager, mock_docker_api, mock_service_creator, fake_metadata_resolver, anything): - # Install the initial version - package_manager.install('test-package=1.6.0') + package_manager.create_package_manifest("default_manifest", from_json=None) - # Mock the metadata for the updated version - updated_manifest = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest'] - updated_components = fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['components'] + mock_echo.assert_called_once_with("Default Manifest creation is not allowed by user") - # Update the metadata resolver to return the updated version metadata - fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['manifest'] = updated_manifest - fake_metadata_resolver.metadata_store['Azure/docker-test']['1.6.0']['components'] = updated_components +def test_create_package_manifest_existing_package(package_manager): + """Test case for creating a manifest with an existing package.""" - # Mock service creator methods - mock_service_creator.create.side_effect = lambda pkg, **opts: print(f'Creating service for {pkg.name}') - mock_service_creator.remove.side_effect = lambda pkg, **opts: print(f'Removing service for {pkg.name}') - mock_service_creator.generate_shutdown_sequence_files.side_effect = lambda pkgs: print(f'Generating shutdown sequence files for {pkgs}') + with patch('os.path.exists', side_effect=[False, True]), \ + patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ + patch('click.echo') as mock_echo: - edit_path = '/var/lib/sonic-package-manager/manifests/test-package.edit' - manifest_path = '/var/lib/sonic-package-manager/manifests/test-package' + package_manager.create_package_manifest("test-package", from_json=None) - with mock.patch('os.rename', side_effect=lambda src, dest: None): - package_manager.update('test-package') + mock_echo.assert_called_once_with("Error: A package with the same name test-package is already installed") - # Assertions for service creator calls - mock_service_creator.remove.assert_called_once_with(anything, deregister_feature=False) +def test_create_package_manifest_existing_manifest(package_manager): + """Test case for creating a manifest with an existing manifest file.""" + + with patch('os.path.exists', return_value=True), \ + patch('click.echo') as mock_echo: + + package_manager.create_package_manifest("test-manifest", from_json=None) + + mock_echo.assert_called_once_with("Error: Manifest file 'test-manifest' already exists.") + +def test_manifests_create_command(package_manager): + with patch('click.echo') as mock_echo, \ + patch('os.path.exists') as mock_exists, \ + patch('os.mkdir') as mock_mkdir, \ + patch('builtins.open', new_callable=mock_open()), \ + patch('json.dump') as mock_json_dump, \ + patch('json.load') as mock_json_load, \ + patch('sonic_package_manager.manifest.Manifest.marshal') as mock_marshal, \ + patch('sonic_package_manager.manager.PackageManager.is_installed') as mock_is_installed, \ + patch('sonic_package_manager.manager.PackageManager.download_file') as mock_download_file: + + dummy_json = {"package": {"name": "test", "version": "1.0.0"}, "service": {"name": "test"}} + # Setup mocks + mock_exists.return_value = False + mock_is_installed.return_value = False + mock_download_file.return_value = True + mock_marshal.return_value = None + mock_json_load.return_value = dummy_json + + # Run the function + package_manager.create_package_manifest("test_manifest", dummy_json) + + # Assertions + mock_echo.assert_called_with("Manifest 'test_manifest' created successfully.") + + +def test_manifests_update_command(package_manager): + with patch('click.echo') as mock_echo, \ + patch('os.path.exists') as mock_exists, \ + patch('os.mkdir') as mock_mkdir, \ + patch('builtins.open', new_callable=unittest.mock.mock_open) as mock_open, \ + patch('json.dump') as mock_json_dump, \ + patch('json.load') as mock_json_load, \ + patch('sonic_package_manager.manifest.Manifest.marshal') as mock_marshal, \ + patch('sonic_package_manager.manager.PackageManager.is_installed') as mock_is_installed, \ + patch('sonic_package_manager.manager.PackageManager.download_file') as mock_download_file: + + dummy_json = {"package": {"name": "test", "version": "2.0.0"}, "service": {"name": "test"}} + # Setup mocks + mock_exists.return_value = True + mock_is_installed.return_value = True + mock_download_file.return_value = True + mock_marshal.return_value = None + mock_json_load.return_value = dummy_json + + # Run the function + package_manager.update_package_manifest("test_manifest", "dummy_json") + + # Assertions + mock_echo.assert_called_with("Manifest 'test_manifest' updated successfully.") + + +def test_check_manifests_directory_existence(package_manager): + with patch('click.echo') as mock_echo, \ + patch('os.path.exists') as mock_exists: + + mock_exists.return_value = False + result = package_manager.check_manifests_directory_existence() + mock_echo.assert_called_with("Manifests files directory empty") + assert result == False + +def test_delete_package_manifest(package_manager): + with patch('click.echo') as mock_echo, \ + patch('click.prompt') as mock_prompt, \ + patch('os.path.exists') as mock_exists, \ + patch('os.remove') as mock_remove, \ + patch.object(package_manager, 'check_manifests_directory_existence') as mock_check_manifests: + + + # Test case 1: deleting default manifest + package_manager.delete_package_manifest("default_manifest") + mock_echo.assert_called_with("Default Manifest deletion is not allowed") + mock_echo.reset_mock() # Reset the mock for the next test case + + mock_check_manifests.return_value = True + # Test case 2: manifest file doesn't exist + mock_exists.return_value = True + mock_exists.side_effect = lambda x: False if x.endswith("test_manifest") else True + package_manager.delete_package_manifest("test_manifest") + mock_echo.assert_called_with("Error: Manifest file 'test_manifest' not found.") + mock_echo.reset_mock() + + # Test case 3: user confirms deletion + mock_exists.side_effect = lambda x: True if x.endswith("test_manifest") else False + mock_prompt.return_value = "y" + package_manager.delete_package_manifest("test_manifest") + #mock_remove.assert_called_with("MANIFEST_LOCATION/test_manifest") + mock_echo.assert_called_with("Manifest 'test_manifest' deleted successfully.") + mock_echo.reset_mock() + + # Test case 4: user cancels deletion + mock_prompt.return_value = "n" + package_manager.delete_package_manifest("test_manifest") + mock_echo.assert_called_with("Deletion cancelled.") + mock_echo.reset_mock() + +def test_show_package_manifest(package_manager): + with patch('click.echo') as mock_echo, \ + patch('os.path.exists') as mock_exists, \ + patch('builtins.open', unittest.mock.mock_open()) as mock_open, \ + patch.object(package_manager, 'check_manifests_directory_existence') as mock_check_manifests, \ + patch('json.load') as mock_json_load: + + mock_exists.return_value = True + mock_exists.side_effect = lambda x: True if x.endswith("test_manifest") else False + + dummy_json = {"package": {"name": "test", "version": "2.0.0"}, "service": {"name": "test"}} + mock_json_load.return_value = dummy_json + + package_manager.show_package_manifest("test_manifest") + mock_echo.assert_called_with(json.dumps(dummy_json, indent=4)) + +def test_list_package_manifest(package_manager): + with patch('click.echo') as mock_echo, \ + patch('os.path.exists') as mock_exists, \ + patch('os.listdir') as mock_listdir: + + # Test case 1: manifest directory doesn't exist + mock_exists.return_value = False + package_manager.list_package_manifest() + mock_echo.assert_called_with("Manifests files directory empty") + + # Test case 2: no custom local manifest files found + mock_exists.return_value = True + mock_listdir.return_value = [] + package_manager.list_package_manifest() + mock_echo.assert_called_with("No custom local manifest files found.") + + # Test case 3: custom local manifest files found + mock_listdir.return_value = ["manifest1.json", "manifest2.json"] + package_manager.list_package_manifest() + mock_echo.assert_any_call("Custom Local Manifest files:") + mock_echo.assert_any_call("- manifest1.json") + mock_echo.assert_any_call("- manifest2.json") + + +def test_download_file_http(package_manager): + fake_remote_url = "http://www.example.com/index.html" + fake_local_path = "local_path" + with patch("requests.get") as mock_requests_get: + with patch("builtins.open", mock_open()) as mock_file: + package_manager.download_file(fake_remote_url, fake_local_path) + mock_requests_get.assert_called_once_with(fake_remote_url, stream=True) + mock_file.assert_called_once_with("local_path", "wb") + + +def test_download_file_scp(package_manager): + fake_remote_url = "scp://admin@10.x.x.x:/home/admin/sec_update.json" + fake_local_path = "local_path" + + with patch("paramiko.SSHClient") as mock_ssh_client: + with patch("paramiko.AutoAddPolicy"): + with patch("paramiko.SCPClient") as mock_scp_client: + package_manager.download_file(fake_remote_url, fake_local_path) + + mock_ssh_client.assert_called_once() + mock_ssh_client.return_value.set_missing_host_key_policy.assert_called_once() + mock_ssh_client.return_value.connect.assert_called_once_with( + "10.x.x.x.", + username="admin", + password=None + ) + mock_scp_client.assert_called_once_with(mock_ssh_client.return_value.get_transport()) + mock_scp_client.return_value.get.assert_called_once_with( + "/home/admin/sec_update.json", + "local_path" + ) + +def test_download_file_scp(package_manager): + fake_remote_url = "scp://admin@10.x.x.x:/home/admin/sec_update.json" + fake_local_path = "local_path" + + with patch("paramiko.SSHClient") as mock_ssh_client: + mock_transport = mock_ssh_client.return_value.get_transport.return_value + with patch("scp.SCPClient") as mock_scp_client: + with patch("getpass.getpass", return_value="test_password"): + package_manager.download_file(fake_remote_url, fake_local_path) + + mock_ssh_client.assert_called_once() + mock_ssh_client.return_value.set_missing_host_key_policy.assert_called_once() + mock_ssh_client.return_value.connect.assert_called_once_with( + "10.x.x.x", + username="admin", + password="test_password" + ) + +def test_download_file_sftp(package_manager): + fake_remote_url = "sftp://admin@10.x.x.x:/home/admin/sec_update.json" + fake_local_path = "local_path" + + with patch("paramiko.SSHClient") as mock_ssh_client: + with patch("paramiko.SFTPClient.from_transport") as mock_sftp_from_transport: + with patch("getpass.getpass", return_value="test_password"): + package_manager.download_file(fake_remote_url, fake_local_path) + + mock_ssh_client.assert_called_once() + mock_ssh_client.return_value.set_missing_host_key_policy.assert_called_once() + mock_ssh_client.return_value.connect.assert_called_once_with( + "10.x.x.x", + username="admin", + password="test_password" + ) diff --git a/tests/sonic_package_manager/test_manifest.py b/tests/sonic_package_manager/test_manifest.py index 009895991a..eb1777a78e 100644 --- a/tests/sonic_package_manager/test_manifest.py +++ b/tests/sonic_package_manager/test_manifest.py @@ -1,9 +1,11 @@ #!/usr/bin/env python import pytest +import json +from unittest.mock import patch, mock_open from sonic_package_manager.constraint import ComponentConstraints -from sonic_package_manager.manifest import Manifest, ManifestError +from sonic_package_manager.manifest import Manifest, ManifestError, MANIFESTS_LOCATION def test_manifest_v1_defaults(): @@ -85,3 +87,20 @@ def test_manifest_v1_unmarshal(): for key, section in manifest_json_input.items(): for field, value in section.items(): assert manifest_json[key][field] == value + +@patch("sonic_package_manager.manifest.open", new_callable=mock_open) +def test_get_manifest_from_local_file_existing_manifest(mock_open, sonic_fs): + # Create a mock manifest file + manifest_name = "test_manifest.json" + manifest_content = {"package": {"name": "test_package", "version": "1.0.0"}, + "service": {"name": "test_service"}} + mock_open.return_value.__enter__.return_value.read.return_value = json.dumps(manifest_content) + sonic_fs.create_dir(MANIFESTS_LOCATION) + + # Call the function + desired_dict = Manifest.get_manifest_from_local_file(manifest_name) + + desired_output = {'Tag': 'master', 'com': {'azure': {'sonic': {'manifest': '{\n "package": {\n "name": "test_manifest.json",\n "version": "1.0.0"\n },\n "service": {\n "name": "test_manifest.json"\n }\n}'}}}} + + # Check if the returned dictionary matches the expected structure + assert desired_dict == desired_output diff --git a/tests/sonic_package_manager/test_metadata.py b/tests/sonic_package_manager/test_metadata.py index bcc59d00ad..1c0dc82227 100644 --- a/tests/sonic_package_manager/test_metadata.py +++ b/tests/sonic_package_manager/test_metadata.py @@ -2,13 +2,15 @@ import json import contextlib -from unittest.mock import Mock, MagicMock, patch, mock_open +from unittest.mock import Mock, MagicMock, patch +import tempfile +import os import pytest from sonic_package_manager.database import PackageEntry from sonic_package_manager.errors import MetadataError -from sonic_package_manager.manifest import Manifest -from sonic_package_manager.metadata import MetadataResolver, Metadata +from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME +from sonic_package_manager.metadata import MetadataResolver from sonic_package_manager.version import Version @@ -86,70 +88,102 @@ def test_metadata_construction(manifest_str): }) assert metadata.yang_modules == ['TEST', 'TEST 2'] +@pytest.fixture +def temp_manifest_dir(): + with tempfile.TemporaryDirectory() as temp_dir: + yield temp_dir -@patch("sonic_package_manager.metadata.os.path.exists", return_value=True) -@patch("sonic_package_manager.metadata.open", create=True) -def test_get_manifest_from_local_file(mock_open, mock_path_exists, mock_docker_api, mock_registry_resolver): - # Setting up mock file content with required fields - mock_file_content = { - "package": { - "name": "test-package", - "version": "1.0.0", - }, - "service": { - "name": "test-package", - "asic-service": False, - "host-service": True, - }, - "container": { - "privileged": True, - }, - } - mock_open.return_value.__enter__.return_value.read.return_value = json.dumps(mock_file_content) +@pytest.fixture +def temp_tarball(temp_manifest_dir): + tarball_path = os.path.join(temp_manifest_dir, 'image.tar') + # Create an empty tarball file for testing + open(tarball_path, 'w').close() + yield tarball_path + +def test_metadata_resolver_local_with_name_and_use_local_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + # Patching the get_manifest_from_local_file method to avoid FileNotFoundError + with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: + # Setting the side_effect to None to simulate the absence of a manifest file + mock_get_manifest.side_effect = None + with contextlib.suppress(MetadataError): + metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=False) + +def test_metadata_resolver_local_manifest_file_not_exist(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + # Patching the get_manifest_from_local_file method to avoid FileNotFoundError + with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: + # Setting the side_effect to None to simulate the absence of a manifest file + mock_get_manifest.side_effect = None + with pytest.raises(MetadataError): + metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=False) + +def test_metadata_resolver_tarball_with_use_local_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + # Patching the get_manifest_from_local_file method to avoid FileNotFoundError + with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: + # Setting the side_effect to None to simulate the absence of a manifest file + mock_get_manifest.side_effect = None + with pytest.raises(MetadataError): + metadata_resolver.from_tarball('image.tar', use_local_manifest=True, name='test_manifest') + +def test_metadata_resolver_no_name_and_no_metadata_in_labels_for_remote(mock_registry_resolver, mock_docker_api): + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + # Mocking the registry resolver's get_registry_for method to return a MagicMock + mock_registry_resolver.get_registry_for = MagicMock(return_value=Mock()) + with pytest.raises(TypeError): + metadata_resolver.from_registry('test-repository', '1.2.0') - # Creating a mock MetadataResolver +def test_metadata_resolver_tarball_with_use_local_manifest_true(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + # Patching the get_manifest_from_local_file method to avoid FileNotFoundError + with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: + # Setting the side_effect to None to simulate the absence of a manifest file + mock_get_manifest.side_effect = None + with pytest.raises(MetadataError): + metadata_resolver.from_tarball('image.tar', use_local_manifest=True) + +def test_metadata_resolver_no_metadata_in_labels_for_tarball(mock_registry_resolver, mock_docker_api): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + with pytest.raises(FileNotFoundError): + metadata_resolver.from_tarball('image.tar') - # Mocking necessary dependencies in the MetadataResolver instance - metadata_resolver.docker.labels = Mock(return_value={"com.azure.sonic.manifest": "mocked_manifest_labels"}) - # Testing with an existing local manifest - metadata = metadata_resolver.from_local('test-image', use_local_manifest=True, name='test-package', use_edit=False) +def test_metadata_resolver_local_with_name_and_use_edit(mock_registry_resolver, mock_docker_api, temp_manifest_dir, sonic_fs): + with patch('builtins.open') as mock_open, \ + patch('json.loads') as mock_json_loads: + sonic_fs.create_dir(MANIFESTS_LOCATION) # Create the directory using sonic_fs fixture + mock_open.side_effect = FileNotFoundError # Simulate FileNotFoundError when opening the manifest file + mock_json_loads.side_effect = ValueError # Simulate ValueError when parsing JSON - assert isinstance(metadata, Metadata) + # Create the default manifest file + default_manifest_path = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) + sonic_fs.create_file(default_manifest_path) + sonic_fs.create_file(os.path.join(MANIFESTS_LOCATION, "test_manifest.edit")) + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + with pytest.raises(FileNotFoundError): + metadata = metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=True) + mock_open.assert_called_with(os.path.join(MANIFESTS_LOCATION, 'test_manifest.edit'), 'r') + mock_json_loads.assert_not_called() # Ensure json.loads is not called -def test_get_manifest_from_local_file2(capsys): - metadata_resolver = MetadataResolver(None, None) # Replace None with appropriate mocks +def test_metadata_resolver_local_with_name_and_default_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir, sonic_fs): + with patch('builtins.open') as mock_open, \ + patch('json.loads') as mock_json_loads: + sonic_fs.create_dir(MANIFESTS_LOCATION) # Create the directory using sonic_fs fixture + mock_open.side_effect = FileNotFoundError # Simulate FileNotFoundError when opening the manifest file + mock_json_loads.side_effect = ValueError # Simulate ValueError when parsing JSON - with patch('os.path.exists', return_value=True), \ - patch('os.mkdir'), \ - patch('builtins.open', mock_open(read_data=json.dumps({"package": {"name": "test-package"}}))): - # Test when manifest file exists - manifest = metadata_resolver.get_manifest_from_local_file('test-package') - assert manifest is not None + # Create the default manifest file + default_manifest_path = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) + sonic_fs.create_file(default_manifest_path) - with patch('os.path.exists', return_value=False), \ - patch('os.mkdir'), \ - patch('builtins.open', mock_open()): - # Test when manifest file does not exist - manifest = metadata_resolver.get_manifest_from_local_file('non-existent-package') - captured = capsys.readouterr() - assert captured.out.strip() == "Local Manifest file /var/lib/sonic-package-manager/manifests/non-existent-package does not exists" - assert manifest is None + metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) + with pytest.raises(FileNotFoundError): + metadata = metadata_resolver.from_local('image', use_local_manifest=False, name='test_manifest', use_edit=True) - manifest_data = {"package": {"name": "test-package"}, "service": {"name": "test-package"}} - with patch('os.path.exists', return_value=True), \ - patch('os.mkdir'), \ - patch('builtins.open', mock_open(read_data=json.dumps(manifest_data))), \ - patch('json.load', return_value=manifest_data), \ - patch('json.dump') as mock_json_dump: - # Test when custom_name is provided - manifest = metadata_resolver.get_manifest_from_local_file('test-package', custom_name='custom-name') + mock_open.assert_called_with(os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME), 'r') + mock_json_loads.assert_not_called() # Ensure json.loads is not called - captured = capsys.readouterr() - print("Captured Output:", captured.out) - # Assert that the manifest is not None - assert manifest is not None diff --git a/tests/test_sonic_installer.py b/tests/test_sonic_installer.py index 9e8438a7fc..648afa5030 100644 --- a/tests/test_sonic_installer.py +++ b/tests/test_sonic_installer.py @@ -86,6 +86,8 @@ def rootfs_path_mock(path): call(["sh", "-c", f"echo 'DOCKER_OPTS=\"$DOCKER_OPTS {' '.join(dockerd_opts)}\"' >> {mounted_image_folder}/etc/default/docker"]), # dockerd started with added options as host dockerd call(["chroot", mounted_image_folder, "/usr/lib/docker/docker.sh", "start"]), call(["cp", "/var/lib/sonic-package-manager/packages.json", f"{mounted_image_folder}/tmp/packages.json"]), + call(["mkdir", "-p", "/var/lib/sonic-package-manager/manifests"]), + call(["cp", "-arf", "/var/lib/sonic-package-manager/manifests", f"{mounted_image_folder}/var/lib/sonic-package-manager"]), call(["touch", f"{mounted_image_folder}/tmp/docker.sock"]), call(["mount", "--bind", "/var/run/docker.sock", f"{mounted_image_folder}/tmp/docker.sock"]), call(["cp", f"{mounted_image_folder}/etc/resolv.conf", "/tmp/resolv.conf.backup"]), From fffeda1ecaedc864a7792b76409eaf6065feeb7d Mon Sep 17 00:00:00 2001 From: sg893052 Date: Thu, 9 May 2024 11:43:13 -0700 Subject: [PATCH 3/4] Thrid Party Container Management using Sonic Package Manager - followup2 --- sonic_package_manager/manager.py | 40 +++-------- sonic_package_manager/manifest.py | 75 ++------------------ tests/sonic_package_manager/test_cli.py | 6 +- tests/sonic_package_manager/test_manager.py | 39 +++------- tests/sonic_package_manager/test_metadata.py | 10 ++- 5 files changed, 32 insertions(+), 138 deletions(-) diff --git a/sonic_package_manager/manager.py b/sonic_package_manager/manager.py index bba93973c3..ccba20b629 100644 --- a/sonic_package_manager/manager.py +++ b/sonic_package_manager/manager.py @@ -73,7 +73,7 @@ import paramiko import urllib.parse from scp import SCPClient -from sonic_package_manager.manifest import Manifest, DEFAULT_MANIFEST, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE +from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE LOCAL_JSON="/tmp/local_json" @contextlib.contextmanager @@ -813,14 +813,16 @@ def migrate_package(old_package_entry, package_source = self.get_package_source(package_ref=new_package_ref) package = package_source.get_package() new_package_default_version = package.manifest['package']['version'] - if old_package.version >= new_package_default_version: + if old_package.version > new_package_default_version: log.info(f'{old_package.name} package version is lower ' - f'then or equal to the default in new image: ' - f'{old_package.version} >= {new_package_default_version}') + f'then the default in new image: ' + f'{old_package.version} > {new_package_default_version}') new_package.version = old_package.version migrate_package(old_package, new_package) else: - self.install(f'{new_package.name}={new_package_default_version}') + #self.install(f'{new_package.name}={new_package_default_version}') + repo_tag_formed="{}:{}".format(new_package.repository, new_package.default_reference) + self.install(None,repo_tag_formed,name=new_package.name) else: # No default version and package is not installed. # Migrate old package same version. @@ -870,7 +872,7 @@ def get_package_source(self, if package_expression: ref = parse_reference_expression(package_expression) - return self.get_package_source(package_ref=ref) + return self.get_package_source(package_ref=ref, name=name) elif repository_reference: repo_ref = utils.DockerReference.parse(repository_reference) repository = repo_ref['name'] @@ -1189,12 +1191,6 @@ def create_package_manifest(self, name, from_json): click.echo("Error: Manifest file '{}' already exists.".format(name)) return - #Creation of default manifest file in case the file does not exist - if not os.path.exists(MANIFESTS_LOCATION): - os.mkdir(MANIFESTS_LOCATION) - if not os.path.exists(DEFAULT_MANIFEST_FILE): - with open(DEFAULT_MANIFEST_FILE, 'w') as file: - json.dump(DEFAULT_MANIFEST, file, indent=4) if from_json: ret = self.download_file(from_json, LOCAL_JSON) @@ -1217,19 +1213,12 @@ def create_package_manifest(self, name, from_json): json.dump(data, file, indent=4) click.echo(f"Manifest '{name}' created successfully.") - def check_manifests_directory_existence(self): - if not os.path.exists(MANIFESTS_LOCATION): - click.echo("Manifests files directory empty") - return False def update_package_manifest(self, name, from_json): if name == "default_manifest": click.echo("Default Manifest updation is not allowed") return - ret = self.check_manifests_directory_existence() - if ret is False: - return original_file = os.path.join(MANIFESTS_LOCATION, name) if not os.path.exists(original_file): click.echo(f'Local Manifest file for {name} does not exists to update') @@ -1267,11 +1256,8 @@ def delete_package_manifest(self, name): if name == "default_manifest": click.echo("Default Manifest deletion is not allowed") return - ret = self.check_manifests_directory_existence() - if ret is False: - return # Check if the manifest file exists - mfile_name = "{}{}".format(MANIFESTS_LOCATION, name) + mfile_name = "{}/{}".format(MANIFESTS_LOCATION, name) if not os.path.exists(mfile_name): click.echo("Error: Manifest file '{}' not found.".format(name)) return @@ -1285,10 +1271,7 @@ def delete_package_manifest(self, name): return def show_package_manifest(self, name): - ret = self.check_manifests_directory_existence() - if ret is False: - return - mfile_name = "{}{}".format(MANIFESTS_LOCATION, name) + mfile_name = "{}/{}".format(MANIFESTS_LOCATION, name) edit_file_name = "{}.edit".format(mfile_name) if os.path.exists(edit_file_name): mfile_name = edit_file_name @@ -1298,9 +1281,6 @@ def show_package_manifest(self, name): click.echo(json.dumps(data, indent=4)) def list_package_manifest(self): - ret = self.check_manifests_directory_existence() - if ret is False: - return # Get all files in the manifest location manifest_files = os.listdir(MANIFESTS_LOCATION) if not manifest_files: diff --git a/sonic_package_manager/manifest.py b/sonic_package_manager/manifest.py index 0c00cb9093..f102b0178c 100644 --- a/sonic_package_manager/manifest.py +++ b/sonic_package_manager/manifest.py @@ -10,73 +10,12 @@ ) from sonic_package_manager.errors import ManifestError from sonic_package_manager.version import Version - +from sonic_package_manager.database import BASE_LIBRARY_PATH import os import json -DEFAULT_MANIFEST = { - "version": "1.0.0", - "package": { - "version": "1.0.0", - "name": "default_manifest", - "description": "", - "base-os": {}, - "depends": [], - "breaks": [], - "init-cfg": {}, - "changelog": {}, - "debug-dump": "" - }, - "service": { - "name": "default_manifest", - "requires": [], - "requisite": [], - "wanted-by": [], - "after": [], - "before": [], - "dependent": [], - "dependent-of": [], - "post-start-action": "", - "pre-shutdown-action": "", - "asic-service": False, - "host-service": True, - "delayed": False, - "check_up_status": False, - "warm-shutdown": { - "after": [], - "before": [] - }, - "fast-shutdown": { - "after": [], - "before": [] - }, - "syslog": { - "support-rate-limit": False - } - }, - "container": { - "privileged": False, - "entrypoint": "", - "volumes": [], - "mounts": [], - "environment": {}, - "tmpfs": [] - }, - "processes": [], - "cli": { - "mandatory": False, - "show": [], - "config": [], - "clear": [], - "auto-generate-show": False, - "auto-generate-config": False, - "auto-generate-show-source-yang-modules": [], - "auto-generate-config-source-yang-modules": [] - } -} -MANIFESTS_LOCATION = "/var/lib/sonic-package-manager/manifests/" -DEFAULT_MANIFEST_NAME = "default_manifest" -DEFAULT_MANIFEST_FILE = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) +MANIFESTS_LOCATION = os.path.join(BASE_LIBRARY_PATH, "manifests") +DEFAULT_MANIFEST_FILE = os.path.join(BASE_LIBRARY_PATH, "default_manifest") class ManifestSchema: """ ManifestSchema class describes and provides marshalling @@ -318,12 +257,6 @@ def unmarshal(self) -> Dict: def get_manifest_from_local_file(name): - if not os.path.exists(MANIFESTS_LOCATION): - os.mkdir(MANIFESTS_LOCATION) - if not os.path.exists(DEFAULT_MANIFEST_FILE): - with open(DEFAULT_MANIFEST_FILE, 'w') as file: - json.dump(DEFAULT_MANIFEST, file, indent=4) - if '.edit' in name: actual_name = name.split('.edit')[0] else: @@ -351,9 +284,9 @@ def get_manifest_from_local_file(name): 'azure': { 'sonic': { 'manifest': json_str - } } } } + } return desired_dict diff --git a/tests/sonic_package_manager/test_cli.py b/tests/sonic_package_manager/test_cli.py index f93632a0c0..c66a7eb165 100644 --- a/tests/sonic_package_manager/test_cli.py +++ b/tests/sonic_package_manager/test_cli.py @@ -113,7 +113,7 @@ def test_manifests_update_command_error_handling(package_manager): patch('os.geteuid', return_value=0): result = runner.invoke(main.manifests.commands['update'], ['non-existent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) - assert 'Manifests files directory empty\n' in result.output + assert 'Local Manifest file for non-existent-manifest does not exists to update\n' in result.output assert result.exit_code == 0 def test_manifests_delete_command_deletion_cancelled(package_manager): @@ -140,7 +140,7 @@ def test_manifests_list_command_no_manifests(package_manager): result = runner.invoke(main.manifests.commands['list'], [], obj=package_manager) # Check if the appropriate message is present in the result output - assert 'Manifests files directory empty\n' in result.output + assert 'No custom local manifest files found.\n' in result.output def test_manifests_show_command_file_not_found(package_manager): runner = CliRunner() @@ -151,7 +151,7 @@ def test_manifests_show_command_file_not_found(package_manager): result = runner.invoke(main.manifests.commands['show'], ['nonexistent-manifest'], obj=package_manager) # Check if the appropriate error message is present in the result output - assert 'Manifests files directory empty\n' in result.output + assert 'No custom local manifest files found.\n' in result.output def test_manifests_command(): """ Test case for "sonic-package-manager manifests" """ diff --git a/tests/sonic_package_manager/test_manager.py b/tests/sonic_package_manager/test_manager.py index a8fbce77c4..4d6ab4c4f9 100644 --- a/tests/sonic_package_manager/test_manager.py +++ b/tests/sonic_package_manager/test_manager.py @@ -354,10 +354,10 @@ def test_manager_migration(package_manager, fake_db_for_migration): call('test-package-3=1.6.0'), # test-package-4 was not present in DB at all, but it is present and installed in # fake_db_for_migration, thus asserting that it is going to be installed. - call('test-package-4=1.5.0'), + call(None, 'Azure/docker-test-4:1.5.0', name='test-package-4'), # test-package-5 1.5.0 was installed in fake_db_for_migration but the default # in current db is 1.9.0, assert that migration will install the newer version. - call('test-package-5=1.9.0'), + call(None, 'Azure/docker-test-5:1.9.0', name='test-package-5'), # test-package-6 2.0.0 was installed in fake_db_for_migration but the default # in current db is 1.5.0, assert that migration will install the newer version. call('test-package-6=2.0.0')], @@ -385,12 +385,12 @@ def save(self, named): return DockerClient(dockerd_sock) -#def test_manager_migration_dockerd(package_manager, fake_db_for_migration, mock_docker_api): - #package_manager.install = Mock() - #package_manager.get_docker_client = Mock(side_effect=mock_get_docker_client) - #package_manager.migrate_packages(fake_db_for_migration, '/var/run/docker.sock') - #package_manager.get_docker_client.assert_has_calls([ - #call('/var/run/docker.sock')], any_order=True) +def test_manager_migration_dockerd(package_manager, fake_db_for_migration, mock_docker_api): + package_manager.install = Mock() + package_manager.get_docker_client = Mock(side_effect=mock_get_docker_client) + package_manager.migrate_packages(fake_db_for_migration, '/var/run/docker.sock') + package_manager.get_docker_client.assert_has_calls([ + call('/var/run/docker.sock')], any_order=True) def test_create_package_manifest_default_manifest(package_manager): @@ -478,29 +478,18 @@ def test_manifests_update_command(package_manager): mock_echo.assert_called_with("Manifest 'test_manifest' updated successfully.") -def test_check_manifests_directory_existence(package_manager): - with patch('click.echo') as mock_echo, \ - patch('os.path.exists') as mock_exists: - - mock_exists.return_value = False - result = package_manager.check_manifests_directory_existence() - mock_echo.assert_called_with("Manifests files directory empty") - assert result == False def test_delete_package_manifest(package_manager): with patch('click.echo') as mock_echo, \ patch('click.prompt') as mock_prompt, \ patch('os.path.exists') as mock_exists, \ - patch('os.remove') as mock_remove, \ - patch.object(package_manager, 'check_manifests_directory_existence') as mock_check_manifests: - + patch('os.remove') as mock_remove: # Test case 1: deleting default manifest package_manager.delete_package_manifest("default_manifest") mock_echo.assert_called_with("Default Manifest deletion is not allowed") mock_echo.reset_mock() # Reset the mock for the next test case - mock_check_manifests.return_value = True # Test case 2: manifest file doesn't exist mock_exists.return_value = True mock_exists.side_effect = lambda x: False if x.endswith("test_manifest") else True @@ -526,7 +515,6 @@ def test_show_package_manifest(package_manager): with patch('click.echo') as mock_echo, \ patch('os.path.exists') as mock_exists, \ patch('builtins.open', unittest.mock.mock_open()) as mock_open, \ - patch.object(package_manager, 'check_manifests_directory_existence') as mock_check_manifests, \ patch('json.load') as mock_json_load: mock_exists.return_value = True @@ -543,18 +531,13 @@ def test_list_package_manifest(package_manager): patch('os.path.exists') as mock_exists, \ patch('os.listdir') as mock_listdir: - # Test case 1: manifest directory doesn't exist - mock_exists.return_value = False - package_manager.list_package_manifest() - mock_echo.assert_called_with("Manifests files directory empty") - - # Test case 2: no custom local manifest files found + # Test case 1: no custom local manifest files found mock_exists.return_value = True mock_listdir.return_value = [] package_manager.list_package_manifest() mock_echo.assert_called_with("No custom local manifest files found.") - # Test case 3: custom local manifest files found + # Test case 2: custom local manifest files found mock_listdir.return_value = ["manifest1.json", "manifest2.json"] package_manager.list_package_manifest() mock_echo.assert_any_call("Custom Local Manifest files:") diff --git a/tests/sonic_package_manager/test_metadata.py b/tests/sonic_package_manager/test_metadata.py index 1c0dc82227..28861d744d 100644 --- a/tests/sonic_package_manager/test_metadata.py +++ b/tests/sonic_package_manager/test_metadata.py @@ -9,7 +9,7 @@ from sonic_package_manager.database import PackageEntry from sonic_package_manager.errors import MetadataError -from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME +from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE from sonic_package_manager.metadata import MetadataResolver from sonic_package_manager.version import Version @@ -157,8 +157,7 @@ def test_metadata_resolver_local_with_name_and_use_edit(mock_registry_resolver, mock_json_loads.side_effect = ValueError # Simulate ValueError when parsing JSON # Create the default manifest file - default_manifest_path = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) - sonic_fs.create_file(default_manifest_path) + sonic_fs.create_file(DEFAULT_MANIFEST_FILE) sonic_fs.create_file(os.path.join(MANIFESTS_LOCATION, "test_manifest.edit")) metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) @@ -176,14 +175,13 @@ def test_metadata_resolver_local_with_name_and_default_manifest(mock_registry_re mock_json_loads.side_effect = ValueError # Simulate ValueError when parsing JSON # Create the default manifest file - default_manifest_path = os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME) - sonic_fs.create_file(default_manifest_path) + sonic_fs.create_file(DEFAULT_MANIFEST_FILE) metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) with pytest.raises(FileNotFoundError): metadata = metadata_resolver.from_local('image', use_local_manifest=False, name='test_manifest', use_edit=True) - mock_open.assert_called_with(os.path.join(MANIFESTS_LOCATION, DEFAULT_MANIFEST_NAME), 'r') + mock_open.assert_called_with(DEFAULT_MANIFEST_FILE, 'r') mock_json_loads.assert_not_called() # Ensure json.loads is not called From c1416659b423b2cad2ff37a6d95a151dec138875 Mon Sep 17 00:00:00 2001 From: sg893052 Date: Fri, 10 May 2024 04:13:31 -0700 Subject: [PATCH 4/4] Fix Pretest Static Analysis found formats --- sonic_installer/main.py | 2 +- sonic_package_manager/main.py | 38 ++++----- sonic_package_manager/manager.py | 44 +++++----- sonic_package_manager/manifest.py | 1 - sonic_package_manager/metadata.py | 15 ++-- sonic_package_manager/source.py | 4 +- tests/sonic_package_manager/test_cli.py | 81 ++++++++++++------- tests/sonic_package_manager/test_manager.py | 53 ++++-------- tests/sonic_package_manager/test_manifest.py | 15 +++- tests/sonic_package_manager/test_metadata.py | 45 ++++++++--- .../test_service_creator.py | 37 --------- tests/test_sonic_installer.py | 3 +- 12 files changed, 171 insertions(+), 167 deletions(-) diff --git a/sonic_installer/main.py b/sonic_installer/main.py index b099d6d119..c5d3a256f2 100644 --- a/sonic_installer/main.py +++ b/sonic_installer/main.py @@ -375,7 +375,7 @@ def migrate_sonic_packages(bootloader, binary_image_version): docker_started = True run_command_or_raise(["cp", packages_path, os.path.join(new_image_mount, TMP_DIR, packages_file)]) run_command_or_raise(["mkdir", "-p", custom_manifests_path]) - run_command_or_raise(["cp", "-arf", custom_manifests_path , new_image_package_directory_path]) + run_command_or_raise(["cp", "-arf", custom_manifests_path, new_image_package_directory_path]) run_command_or_raise(["touch", os.path.join(new_image_mount, "tmp", DOCKERD_SOCK)]) run_command_or_raise(["mount", "--bind", os.path.join(VAR_RUN_PATH, DOCKERD_SOCK), diff --git a/sonic_package_manager/main.py b/sonic_package_manager/main.py index af3b0ecfb3..d305e3c911 100644 --- a/sonic_package_manager/main.py +++ b/sonic_package_manager/main.py @@ -9,10 +9,6 @@ import click import click_log import tabulate -from urllib.parse import urlparse -import paramiko -import requests -import getpass from natsort import natsorted from sonic_package_manager.database import PackageEntry, PackageDatabase @@ -166,7 +162,7 @@ def repository(ctx): @click.pass_context def manifests(ctx): """ Custom local Manifest management commands. """ - + pass @cli.group() @@ -292,13 +288,12 @@ def changelog(ctx, exit_cli(f'Failed to print package changelog: {err}', fg='red') - @manifests.command('create') @click.pass_context @click.argument('name', type=click.Path()) @click.option('--from-json', type=str, help='specify manifest json file') @root_privileges_required -def create(ctx, name, from_json): +def create_manifest(ctx, name, from_json): """Create a new custom local manifest file.""" manager: PackageManager = ctx.obj @@ -307,17 +302,14 @@ def create(ctx, name, from_json): except Exception as e: click.echo("Error: Manifest {} creation failed - {}".format(name, str(e))) return - -#At the end of sonic-package-manager install, a new manifest file is created with the name. -#At the end of sonic-package-manager uninstall name, this manifest file name and name.edit will be deleted. -#At the end of sonic-package-manager update, we need to mv maniests name.edit to name in case of success, else keep it as such. -#So during sonic-package-manager update, we could take old package from name and new package from edit and at the end, follow 3rd point + + @manifests.command('update') @click.pass_context @click.argument('name', type=click.Path()) @click.option('--from-json', type=str, required=True) @root_privileges_required -def update(ctx, name, from_json): +def update_manifest(ctx, name, from_json): """Update an existing custom local manifest file with new one.""" manager: PackageManager = ctx.obj @@ -327,11 +319,12 @@ def update(ctx, name, from_json): click.echo(f"Error occurred while updating manifest '{name}': {e}") return + @manifests.command('delete') @click.pass_context @click.argument('name', type=click.Path()) @root_privileges_required -def delete(ctx, name): +def delete_manifest(ctx, name): """Delete a custom local manifest file.""" manager: PackageManager = ctx.obj try: @@ -352,6 +345,7 @@ def show_manifest(ctx, name): except FileNotFoundError: click.echo("Manifest file '{}' not found.".format(name)) + @manifests.command('list') @click.pass_context @root_privileges_required @@ -421,8 +415,8 @@ def remove(ctx, name): help='Use locally created custom manifest file. ', hidden=True) @click.option('--name', - type=str, - help='custom name for the package') + type=str, + help='custom name for the package') @add_options(PACKAGE_SOURCE_OPTIONS) @add_options(PACKAGE_COMMON_OPERATION_OPTIONS) @add_options(PACKAGE_COMMON_INSTALL_OPTIONS) @@ -471,13 +465,13 @@ def install(ctx, if use_local_manifest: if not name: - click.echo(f'name argument is not provided to use local manifest') + click.echo('name argument is not provided to use local manifest') return original_file = os.path.join(MANIFESTS_LOCATION, name) if not os.path.exists(original_file): click.echo(f'Local Manifest file for {name} does not exists to install') return - + try: manager.install(package_expr, from_repository, @@ -490,6 +484,14 @@ def install(ctx, except KeyboardInterrupt: exit_cli('Operation canceled by user', fg='red') +# At the end of sonic-package-manager install, a new manifest file is created with the name. +# At the end of sonic-package-manager uninstall name, +# this manifest file name and name.edit will be deleted. +# At the end of sonic-package-manager update, +# we need to mv maniests name.edit to name in case of success, else keep it as such. +# So during sonic-package-manager update, +# we could take old package from name and new package from edit and at the end, follow 3rd point + @cli.command() @add_options(PACKAGE_COMMON_OPERATION_OPTIONS) diff --git a/sonic_package_manager/manager.py b/sonic_package_manager/manager.py index ccba20b629..a052479607 100644 --- a/sonic_package_manager/manager.py +++ b/sonic_package_manager/manager.py @@ -67,14 +67,13 @@ ) import click import json -import os import requests import getpass import paramiko import urllib.parse from scp import SCPClient from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE -LOCAL_JSON="/tmp/local_json" +LOCAL_JSON = "/tmp/local_json" @contextlib.contextmanager def failure_ignore(ignore: bool): @@ -458,9 +457,9 @@ def install_from_source(self, self.database.commit() @under_lock - def update(self, - name: str, - **kwargs): + def update(self, + name: str, + **kwargs): """ Update SONiC Package referenced by name. The update can be forced if force argument is True. @@ -481,7 +480,6 @@ def update(self, click.echo("Package {} is not installed".format(name)) return - def remove_unused_docker_image(self, package): image_id_used = any(entry.image_id == package.image_id for entry in self.database if entry.name != package.name) if not image_id_used: @@ -579,7 +577,7 @@ def upgrade_from_source(self, Raises: PackageManagerError """ - + if update_only: new_package = self.get_installed_package(name, use_edit=True) else: @@ -679,10 +677,10 @@ def upgrade_from_source(self, if not skip_host_plugins: self._install_cli_plugins(new_package) exits.callback(rollback(self._uninstall_cli_plugin, new_package)) - + if old_package.image_id != new_package.image_id: self.remove_unused_docker_image(old_package) - + exits.pop_all() except Exception as err: raise PackageUpgradeError(f'Failed to upgrade {new_package.name}: {err}') @@ -697,7 +695,7 @@ def upgrade_from_source(self, if update_only: manifest_path = os.path.join(MANIFESTS_LOCATION, name) edit_path = os.path.join(MANIFESTS_LOCATION, name + ".edit") - os.rename(edit_path,manifest_path) + os.rename(edit_path, manifest_path) @under_lock @opt_check @@ -820,9 +818,9 @@ def migrate_package(old_package_entry, new_package.version = old_package.version migrate_package(old_package, new_package) else: - #self.install(f'{new_package.name}={new_package_default_version}') - repo_tag_formed="{}:{}".format(new_package.repository, new_package.default_reference) - self.install(None,repo_tag_formed,name=new_package.name) + # self.install(f'{new_package.name}={new_package_default_version}') + repo_tag_formed = "{}:{}".format(new_package.repository, new_package.default_reference) + self.install(None, repo_tag_formed, name=new_package.name) else: # No default version and package is not installed. # Migrate old package same version. @@ -1108,12 +1106,12 @@ def download_file(self, url, local_path): remote_path = parsed_url.path supported_protocols = ['http', 'https', 'scp', 'sftp'] - #clear the temporary local file + # clear the temporary local file if os.path.exists(local_path): os.remove(local_path) if not protocol: - #check for local file + # check for local file if os.path.exists(url): os.rename(url, local_path) return True @@ -1191,7 +1189,6 @@ def create_package_manifest(self, name, from_json): click.echo("Error: Manifest file '{}' already exists.".format(name)) return - if from_json: ret = self.download_file(from_json, LOCAL_JSON) if ret is False: @@ -1202,10 +1199,10 @@ def create_package_manifest(self, name, from_json): data = {} with open(from_json, 'r') as file: data = json.load(file) - #Validate with manifest scheme + # Validate with manifest scheme Manifest.marshal(data) - #Make sure the 'name' is overwritten into the dict + # Make sure the 'name' is overwritten into the dict data['package']['name'] = name data['service']['name'] = name @@ -1213,7 +1210,6 @@ def create_package_manifest(self, name, from_json): json.dump(data, file, indent=4) click.echo(f"Manifest '{name}' created successfully.") - def update_package_manifest(self, name, from_json): if name == "default_manifest": click.echo("Default Manifest updation is not allowed") @@ -1223,7 +1219,7 @@ def update_package_manifest(self, name, from_json): if not os.path.exists(original_file): click.echo(f'Local Manifest file for {name} does not exists to update') return - #download json file from remote/local path + # download json file from remote/local path ret = self.download_file(from_json, LOCAL_JSON) if ret is False: return @@ -1232,10 +1228,10 @@ def update_package_manifest(self, name, from_json): with open(from_json, 'r') as file: data = json.load(file) - #Validate with manifest scheme + # Validate with manifest scheme Manifest.marshal(data) - #Make sure the 'name' is overwritten into the dict + # Make sure the 'name' is overwritten into the dict data['package']['name'] = name data['service']['name'] = name @@ -1246,8 +1242,8 @@ def update_package_manifest(self, name, from_json): json.dump(data, edit_file, indent=4) click.echo(f"Manifest '{name}' updated successfully.") else: - #If package is not installed, - ## update the name file directly + # If package is not installed, + # update the name file directly with open(original_file, 'w') as orig_file: json.dump(data, orig_file, indent=4) click.echo(f"Manifest '{name}' updated successfully.") diff --git a/sonic_package_manager/manifest.py b/sonic_package_manager/manifest.py index f102b0178c..bc156f102c 100644 --- a/sonic_package_manager/manifest.py +++ b/sonic_package_manager/manifest.py @@ -289,4 +289,3 @@ def get_manifest_from_local_file(name): } } return desired_dict - diff --git a/sonic_package_manager/metadata.py b/sonic_package_manager/metadata.py index 873f20b82d..6485a10782 100644 --- a/sonic_package_manager/metadata.py +++ b/sonic_package_manager/metadata.py @@ -5,7 +5,6 @@ import json import tarfile from typing import Dict, List, Optional -import os from sonic_package_manager import utils from sonic_package_manager.errors import MetadataError from sonic_package_manager.logger import log @@ -64,7 +63,8 @@ def __init__(self, docker, registry_resolver): self.docker = docker self.registry_resolver = registry_resolver - def from_local(self, image: str, use_local_manifest: bool = False, name: Optional[str] = None, use_edit: bool = False) -> Metadata: + def from_local(self, image: str, use_local_manifest: bool = False, + name: Optional[str] = None, use_edit: bool = False) -> Metadata: """ Reads manifest from locally installed docker image. Args: @@ -88,7 +88,7 @@ def from_local(self, image: str, use_local_manifest: bool = False, name: Optiona if name: labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels') + raise MetadataError('No manifest found in image labels') else: raise MetadataError('No manifest found in image labels') @@ -125,7 +125,7 @@ def from_registry(self, raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels') + raise MetadataError('No manifest found in image labels') return self.from_labels(labels) def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: Optional[str] = None) -> Metadata: @@ -149,10 +149,11 @@ def from_tarball(self, image_path: str, use_local_manifest: bool = False, name: labels = image_config['config'].get('Labels') if labels is None or len(labels) == 0 or 'com.azure.sonic.manifest' not in labels: if name is None: - raise MetadataError('The name(custom) option is required as there is no metadata found in image labels') + raise MetadataError('The name(custom) option is \ + required as there is no metadata found in image labels') labels = Manifest.get_manifest_from_local_file(name) if labels is None: - raise MetadataError('No manifest found in image labels') + raise MetadataError('No manifest found in image labels') return self.from_labels(labels) @classmethod @@ -202,5 +203,5 @@ def from_labels(cls, labels: Dict[str, str]) -> Metadata: log.debug(f"Found YANG modules: {labels_yang_modules.keys()}") else: log.debug("No YANG modules found") - + return Metadata(Manifest.marshal(manifest_dict), components, yang_modules) diff --git a/sonic_package_manager/source.py b/sonic_package_manager/source.py index 5a6d8ff179..2a0f07b0f1 100644 --- a/sonic_package_manager/source.py +++ b/sonic_package_manager/source.py @@ -117,7 +117,9 @@ def __init__(self, def get_metadata(self) -> Metadata: """ Returns manifest read from tarball. """ - return self.metadata_resolver.from_tarball(self.tarball_path, use_local_manifest=self.use_local_manifest, name=self.name) + return self.metadata_resolver.from_tarball(self.tarball_path, + use_local_manifest=self.use_local_manifest, + name=self.name) def install_image(self, package: Package): """ Installs image from local tarball source. """ diff --git a/tests/sonic_package_manager/test_cli.py b/tests/sonic_package_manager/test_cli.py index c66a7eb165..1b7556ae68 100644 --- a/tests/sonic_package_manager/test_cli.py +++ b/tests/sonic_package_manager/test_cli.py @@ -4,8 +4,7 @@ from sonic_package_manager import main -from unittest.mock import patch, Mock, mock_open, MagicMock -import os +from unittest.mock import patch, mock_open, MagicMock MANIFEST_LOCATION = 'fake_manifest_location' DMFILE_NAME = 'fake_dmfile_name' @@ -72,6 +71,7 @@ def test_show_changelog_no_changelog(package_manager): assert result.exit_code == 1 assert result.output == 'Failed to print package changelog: No changelog for package test-package\n' + def test_manifests_create_command_existing_manifest(package_manager): """ Test case for "sonic-package-manager manifests create" with an existing manifest file """ @@ -79,14 +79,18 @@ def test_manifests_create_command_existing_manifest(package_manager): with patch('os.path.exists', side_effect=[True, False]), \ patch('sonic_package_manager.main.PackageManager.is_installed', return_value=False), \ - patch('builtins.open', new_callable=mock_open()) as mock_file, \ + patch('builtins.open', new_callable=mock_open()), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) + result = runner.invoke(main.manifests.commands['create'], + ['test-manifest'], + input=sample_manifest_json, + obj=package_manager) assert 'Error: Manifest file \'test-manifest\' already exists.' in result.output assert result.exit_code == 0 + def test_manifests_create_command_existing_package(package_manager): """ Test case for "sonic-package-manager manifests create" with an existing installed package """ @@ -94,14 +98,18 @@ def test_manifests_create_command_existing_package(package_manager): with patch('os.path.exists', return_value=False), \ patch('sonic_package_manager.main.PackageManager.is_installed', return_value=True), \ - patch('builtins.open', new_callable=mock_open()) as mock_file, \ + patch('builtins.open', new_callable=mock_open()), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], input=sample_manifest_json, obj=package_manager) + result = runner.invoke(main.manifests.commands['create'], + ['test-manifest'], + input=sample_manifest_json, + obj=package_manager) assert 'Error: A package with the same name test-manifest is already installed' in result.output assert result.exit_code == 0 + def test_manifests_update_command_error_handling(package_manager): runner = CliRunner() @@ -111,11 +119,14 @@ def test_manifests_update_command_error_handling(package_manager): patch('json.load', side_effect=lambda x: MagicMock(return_value='{"key": "value"}')), \ patch('json.dump'), \ patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['update'], ['non-existent-manifest', '--from-json', 'fake_json_path'], obj=package_manager) + + result = runner.invoke(main.manifests.commands['update'], + ['non-existent-manifest', '--from-json', 'fake_json_path'], + obj=package_manager) assert 'Local Manifest file for non-existent-manifest does not exists to update\n' in result.output assert result.exit_code == 0 + def test_manifests_delete_command_deletion_cancelled(package_manager): runner = CliRunner() @@ -131,6 +142,7 @@ def test_manifests_delete_command_deletion_cancelled(package_manager): # Check if os.remove is not called when the deletion is cancelled assert not mock_remove.called + def test_manifests_list_command_no_manifests(package_manager): runner = CliRunner() @@ -142,16 +154,6 @@ def test_manifests_list_command_no_manifests(package_manager): # Check if the appropriate message is present in the result output assert 'No custom local manifest files found.\n' in result.output -def test_manifests_show_command_file_not_found(package_manager): - runner = CliRunner() - - with patch('os.path.exists', return_value=False), \ - patch('os.geteuid', return_value=0): - - result = runner.invoke(main.manifests.commands['show'], ['nonexistent-manifest'], obj=package_manager) - - # Check if the appropriate error message is present in the result output - assert 'No custom local manifest files found.\n' in result.output def test_manifests_command(): """ Test case for "sonic-package-manager manifests" """ @@ -166,7 +168,8 @@ def test_manifests_create_command_exception(package_manager): runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.create_package_manifest', side_effect=Exception("Custom error")), \ + with patch('sonic_package_manager.main.PackageManager.create_package_manifest', + side_effect=Exception("Custom error")), \ patch('os.geteuid', return_value=0): result = runner.invoke(main.manifests.commands['create'], ['test-manifest'], obj=package_manager) @@ -174,45 +177,56 @@ def test_manifests_create_command_exception(package_manager): assert 'Error: Manifest test-manifest creation failed - Custom error' in result.output assert result.exit_code == 0 + def test_manifests_update_command_exception(package_manager): """Test case for 'sonic-package-manager manifests update' with an exception during manifest update""" runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.update_package_manifest', side_effect=Exception("Custom error")), \ + with patch('sonic_package_manager.main.PackageManager.update_package_manifest', + side_effect=Exception("Custom error")), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.manifests.commands['update'], ['test-manifest', '--from-json', 'new_manifest.json'], obj=package_manager) + result = runner.invoke(main.manifests.commands['update'], + ['test-manifest', '--from-json', 'new_manifest.json'], + obj=package_manager) assert 'Error occurred while updating manifest \'test-manifest\': Custom error' in result.output assert result.exit_code == 0 + def test_manifests_delete_command_exception(package_manager): """Test case for 'sonic-package-manager manifests delete' with an exception during manifest deletion""" runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.delete_package_manifest', side_effect=Exception("Custom error")), \ + with patch('sonic_package_manager.main.PackageManager.delete_package_manifest', + side_effect=Exception("Custom error")), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.manifests.commands['delete'], ['test-manifest'], obj=package_manager) + result = runner.invoke(main.manifests.commands['delete'], + ['test-manifest'], obj=package_manager) assert "Error: Failed to delete manifest file 'test-manifest'. Custom error" in result.output assert result.exit_code == 0 + def test_manifests_show_command_file_not_found(package_manager): """Test case for 'sonic-package-manager manifests show' with a non-existent manifest file""" runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.show_package_manifest', side_effect=FileNotFoundError()), \ + with patch('sonic_package_manager.main.PackageManager.show_package_manifest', + side_effect=FileNotFoundError()), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.manifests.commands['show'], ['nonexistent_manifest.json'], obj=package_manager) + result = runner.invoke(main.manifests.commands['show'], + ['nonexistent_manifest.json'], obj=package_manager) assert "Manifest file 'nonexistent_manifest.json' not found." in result.output assert result.exit_code == 0 + def test_install_with_local_manifest(package_manager): """Test case for 'install' command with use_local_manifest=True and name provided""" @@ -220,11 +234,14 @@ def test_install_with_local_manifest(package_manager): with patch('os.path.exists', return_value=True), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.install, ['package_name', '--use-local-manifest', '-y'], obj=package_manager) + result = runner.invoke(main.install, + ['package_name', '--use-local-manifest', '-y'], + obj=package_manager) assert 'name argument is not provided to use local manifest' in result.output assert result.exit_code == 0 + def test_install_with_nonexistent_manifest(package_manager): """Test case for 'install' command with use_local_manifest=True and non-existent name provided""" @@ -232,17 +249,22 @@ def test_install_with_nonexistent_manifest(package_manager): with patch('os.path.exists', return_value=False), \ patch('os.geteuid', return_value=0): - result = runner.invoke(main.install, ['package_name', '--use-local-manifest', '--name', 'nonexistent_manifest', '-y'], obj=package_manager) + result = runner.invoke( + main.install, + ['package_name', '--use-local-manifest', '--name', 'nonexistent_manifest', '-y'], + obj=package_manager) assert 'Local Manifest file for nonexistent_manifest does not exists to install' in result.output assert result.exit_code == 0 + def test_update_command_exception(package_manager): """Test case for 'update' command with an exception during package update""" runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.update', side_effect=Exception("Custom error")), \ + with patch('sonic_package_manager.main.PackageManager.update', + side_effect=Exception("Custom error")), \ patch('os.geteuid', return_value=0): result = runner.invoke(main.update, ['package_name'], obj=package_manager) @@ -255,7 +277,8 @@ def test_update_command_keyboard_interrupt(package_manager): runner = CliRunner() - with patch('sonic_package_manager.main.PackageManager.update', side_effect=KeyboardInterrupt()), \ + with patch('sonic_package_manager.main.PackageManager.update', + side_effect=KeyboardInterrupt()), \ patch('os.geteuid', return_value=0): result = runner.invoke(main.update, ['package_name'], obj=package_manager) diff --git a/tests/sonic_package_manager/test_manager.py b/tests/sonic_package_manager/test_manager.py index 4d6ab4c4f9..a3a311ebb2 100644 --- a/tests/sonic_package_manager/test_manager.py +++ b/tests/sonic_package_manager/test_manager.py @@ -2,13 +2,12 @@ import re import unittest -from unittest.mock import Mock, call, patch, mock_open, MagicMock +from unittest.mock import Mock, call, patch, mock_open import pytest import sonic_package_manager from sonic_package_manager.errors import * from sonic_package_manager.version import Version -from sonic_package_manager.manifest import Manifest import json @pytest.fixture(autouse=True) @@ -405,6 +404,7 @@ def test_create_package_manifest_default_manifest(package_manager): mock_echo.assert_called_once_with("Default Manifest creation is not allowed by user") + def test_create_package_manifest_existing_package(package_manager): """Test case for creating a manifest with an existing package.""" @@ -416,6 +416,7 @@ def test_create_package_manifest_existing_package(package_manager): mock_echo.assert_called_once_with("Error: A package with the same name test-package is already installed") + def test_create_package_manifest_existing_manifest(package_manager): """Test case for creating a manifest with an existing manifest file.""" @@ -426,12 +427,13 @@ def test_create_package_manifest_existing_manifest(package_manager): mock_echo.assert_called_once_with("Error: Manifest file 'test-manifest' already exists.") + def test_manifests_create_command(package_manager): with patch('click.echo') as mock_echo, \ patch('os.path.exists') as mock_exists, \ - patch('os.mkdir') as mock_mkdir, \ + patch('os.mkdir'), \ patch('builtins.open', new_callable=mock_open()), \ - patch('json.dump') as mock_json_dump, \ + patch('json.dump'), \ patch('json.load') as mock_json_load, \ patch('sonic_package_manager.manifest.Manifest.marshal') as mock_marshal, \ patch('sonic_package_manager.manager.PackageManager.is_installed') as mock_is_installed, \ @@ -455,9 +457,9 @@ def test_manifests_create_command(package_manager): def test_manifests_update_command(package_manager): with patch('click.echo') as mock_echo, \ patch('os.path.exists') as mock_exists, \ - patch('os.mkdir') as mock_mkdir, \ - patch('builtins.open', new_callable=unittest.mock.mock_open) as mock_open, \ - patch('json.dump') as mock_json_dump, \ + patch('os.mkdir'), \ + patch('builtins.open', new_callable=unittest.mock.mock_open), \ + patch('json.dump'), \ patch('json.load') as mock_json_load, \ patch('sonic_package_manager.manifest.Manifest.marshal') as mock_marshal, \ patch('sonic_package_manager.manager.PackageManager.is_installed') as mock_is_installed, \ @@ -478,12 +480,11 @@ def test_manifests_update_command(package_manager): mock_echo.assert_called_with("Manifest 'test_manifest' updated successfully.") - def test_delete_package_manifest(package_manager): with patch('click.echo') as mock_echo, \ patch('click.prompt') as mock_prompt, \ patch('os.path.exists') as mock_exists, \ - patch('os.remove') as mock_remove: + patch('os.remove'): # Test case 1: deleting default manifest package_manager.delete_package_manifest("default_manifest") @@ -501,7 +502,6 @@ def test_delete_package_manifest(package_manager): mock_exists.side_effect = lambda x: True if x.endswith("test_manifest") else False mock_prompt.return_value = "y" package_manager.delete_package_manifest("test_manifest") - #mock_remove.assert_called_with("MANIFEST_LOCATION/test_manifest") mock_echo.assert_called_with("Manifest 'test_manifest' deleted successfully.") mock_echo.reset_mock() @@ -511,10 +511,11 @@ def test_delete_package_manifest(package_manager): mock_echo.assert_called_with("Deletion cancelled.") mock_echo.reset_mock() + def test_show_package_manifest(package_manager): with patch('click.echo') as mock_echo, \ patch('os.path.exists') as mock_exists, \ - patch('builtins.open', unittest.mock.mock_open()) as mock_open, \ + patch('builtins.open', unittest.mock.mock_open()), \ patch('json.load') as mock_json_load: mock_exists.return_value = True @@ -526,6 +527,7 @@ def test_show_package_manifest(package_manager): package_manager.show_package_manifest("test_manifest") mock_echo.assert_called_with(json.dumps(dummy_json, indent=4)) + def test_list_package_manifest(package_manager): with patch('click.echo') as mock_echo, \ patch('os.path.exists') as mock_exists, \ @@ -560,31 +562,7 @@ def test_download_file_scp(package_manager): fake_local_path = "local_path" with patch("paramiko.SSHClient") as mock_ssh_client: - with patch("paramiko.AutoAddPolicy"): - with patch("paramiko.SCPClient") as mock_scp_client: - package_manager.download_file(fake_remote_url, fake_local_path) - - mock_ssh_client.assert_called_once() - mock_ssh_client.return_value.set_missing_host_key_policy.assert_called_once() - mock_ssh_client.return_value.connect.assert_called_once_with( - "10.x.x.x.", - username="admin", - password=None - ) - mock_scp_client.assert_called_once_with(mock_ssh_client.return_value.get_transport()) - mock_scp_client.return_value.get.assert_called_once_with( - "/home/admin/sec_update.json", - "local_path" - ) - - -def test_download_file_scp(package_manager): - fake_remote_url = "scp://admin@10.x.x.x:/home/admin/sec_update.json" - fake_local_path = "local_path" - - with patch("paramiko.SSHClient") as mock_ssh_client: - mock_transport = mock_ssh_client.return_value.get_transport.return_value - with patch("scp.SCPClient") as mock_scp_client: + with patch("scp.SCPClient"): with patch("getpass.getpass", return_value="test_password"): package_manager.download_file(fake_remote_url, fake_local_path) @@ -596,12 +574,13 @@ def test_download_file_scp(package_manager): password="test_password" ) + def test_download_file_sftp(package_manager): fake_remote_url = "sftp://admin@10.x.x.x:/home/admin/sec_update.json" fake_local_path = "local_path" with patch("paramiko.SSHClient") as mock_ssh_client: - with patch("paramiko.SFTPClient.from_transport") as mock_sftp_from_transport: + with patch("paramiko.SFTPClient.from_transport"): with patch("getpass.getpass", return_value="test_password"): package_manager.download_file(fake_remote_url, fake_local_path) diff --git a/tests/sonic_package_manager/test_manifest.py b/tests/sonic_package_manager/test_manifest.py index eb1777a78e..5eaa2f6053 100644 --- a/tests/sonic_package_manager/test_manifest.py +++ b/tests/sonic_package_manager/test_manifest.py @@ -88,6 +88,7 @@ def test_manifest_v1_unmarshal(): for field, value in section.items(): assert manifest_json[key][field] == value + @patch("sonic_package_manager.manifest.open", new_callable=mock_open) def test_get_manifest_from_local_file_existing_manifest(mock_open, sonic_fs): # Create a mock manifest file @@ -100,7 +101,19 @@ def test_get_manifest_from_local_file_existing_manifest(mock_open, sonic_fs): # Call the function desired_dict = Manifest.get_manifest_from_local_file(manifest_name) - desired_output = {'Tag': 'master', 'com': {'azure': {'sonic': {'manifest': '{\n "package": {\n "name": "test_manifest.json",\n "version": "1.0.0"\n },\n "service": {\n "name": "test_manifest.json"\n }\n}'}}}} + exp_manifest_content = {"package": {"name": "test_manifest.json", "version": "1.0.0"}, + "service": {"name": "test_manifest.json"}} + manifest_string = json.dumps(exp_manifest_content, indent=4) + desired_output = { + 'Tag': 'master', + 'com': { + 'azure': { + 'sonic': { + 'manifest': manifest_string + } + } + } + } # Check if the returned dictionary matches the expected structure assert desired_dict == desired_output diff --git a/tests/sonic_package_manager/test_metadata.py b/tests/sonic_package_manager/test_metadata.py index 28861d744d..f386836a83 100644 --- a/tests/sonic_package_manager/test_metadata.py +++ b/tests/sonic_package_manager/test_metadata.py @@ -9,7 +9,7 @@ from sonic_package_manager.database import PackageEntry from sonic_package_manager.errors import MetadataError -from sonic_package_manager.manifest import Manifest, MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE +from sonic_package_manager.manifest import MANIFESTS_LOCATION, DEFAULT_MANIFEST_FILE from sonic_package_manager.metadata import MetadataResolver from sonic_package_manager.version import Version @@ -88,11 +88,13 @@ def test_metadata_construction(manifest_str): }) assert metadata.yang_modules == ['TEST', 'TEST 2'] + @pytest.fixture def temp_manifest_dir(): with tempfile.TemporaryDirectory() as temp_dir: yield temp_dir + @pytest.fixture def temp_tarball(temp_manifest_dir): tarball_path = os.path.join(temp_manifest_dir, 'image.tar') @@ -100,7 +102,10 @@ def temp_tarball(temp_manifest_dir): open(tarball_path, 'w').close() yield tarball_path -def test_metadata_resolver_local_with_name_and_use_local_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + +def test_metadata_resolver_local_with_name_and_use_local_manifest(mock_registry_resolver, + mock_docker_api, + temp_manifest_dir): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) # Patching the get_manifest_from_local_file method to avoid FileNotFoundError with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: @@ -109,6 +114,7 @@ def test_metadata_resolver_local_with_name_and_use_local_manifest(mock_registry_ with contextlib.suppress(MetadataError): metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=False) + def test_metadata_resolver_local_manifest_file_not_exist(mock_registry_resolver, mock_docker_api, temp_manifest_dir): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) # Patching the get_manifest_from_local_file method to avoid FileNotFoundError @@ -118,7 +124,10 @@ def test_metadata_resolver_local_manifest_file_not_exist(mock_registry_resolver, with pytest.raises(MetadataError): metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=False) -def test_metadata_resolver_tarball_with_use_local_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + +def test_metadata_resolver_tarball_with_use_local_manifest(mock_registry_resolver, + mock_docker_api, + temp_manifest_dir): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) # Patching the get_manifest_from_local_file method to avoid FileNotFoundError with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: @@ -127,6 +136,7 @@ def test_metadata_resolver_tarball_with_use_local_manifest(mock_registry_resolve with pytest.raises(MetadataError): metadata_resolver.from_tarball('image.tar', use_local_manifest=True, name='test_manifest') + def test_metadata_resolver_no_name_and_no_metadata_in_labels_for_remote(mock_registry_resolver, mock_docker_api): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) # Mocking the registry resolver's get_registry_for method to return a MagicMock @@ -134,7 +144,10 @@ def test_metadata_resolver_no_name_and_no_metadata_in_labels_for_remote(mock_reg with pytest.raises(TypeError): metadata_resolver.from_registry('test-repository', '1.2.0') -def test_metadata_resolver_tarball_with_use_local_manifest_true(mock_registry_resolver, mock_docker_api, temp_manifest_dir): + +def test_metadata_resolver_tarball_with_use_local_manifest_true(mock_registry_resolver, + mock_docker_api, + temp_manifest_dir): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) # Patching the get_manifest_from_local_file method to avoid FileNotFoundError with patch('sonic_package_manager.manifest.Manifest.get_manifest_from_local_file') as mock_get_manifest: @@ -143,13 +156,17 @@ def test_metadata_resolver_tarball_with_use_local_manifest_true(mock_registry_re with pytest.raises(MetadataError): metadata_resolver.from_tarball('image.tar', use_local_manifest=True) + def test_metadata_resolver_no_metadata_in_labels_for_tarball(mock_registry_resolver, mock_docker_api): metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) with pytest.raises(FileNotFoundError): metadata_resolver.from_tarball('image.tar') -def test_metadata_resolver_local_with_name_and_use_edit(mock_registry_resolver, mock_docker_api, temp_manifest_dir, sonic_fs): +def test_metadata_resolver_local_with_name_and_use_edit(mock_registry_resolver, + mock_docker_api, + temp_manifest_dir, + sonic_fs): with patch('builtins.open') as mock_open, \ patch('json.loads') as mock_json_loads: sonic_fs.create_dir(MANIFESTS_LOCATION) # Create the directory using sonic_fs fixture @@ -162,12 +179,19 @@ def test_metadata_resolver_local_with_name_and_use_edit(mock_registry_resolver, metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) with pytest.raises(FileNotFoundError): - metadata = metadata_resolver.from_local('image', use_local_manifest=True, name='test_manifest', use_edit=True) + metadata_resolver.from_local('image', + use_local_manifest=True, + name='test_manifest', + use_edit=True) mock_open.assert_called_with(os.path.join(MANIFESTS_LOCATION, 'test_manifest.edit'), 'r') mock_json_loads.assert_not_called() # Ensure json.loads is not called -def test_metadata_resolver_local_with_name_and_default_manifest(mock_registry_resolver, mock_docker_api, temp_manifest_dir, sonic_fs): + +def test_metadata_resolver_local_with_name_and_default_manifest(mock_registry_resolver, + mock_docker_api, + temp_manifest_dir, + sonic_fs): with patch('builtins.open') as mock_open, \ patch('json.loads') as mock_json_loads: sonic_fs.create_dir(MANIFESTS_LOCATION) # Create the directory using sonic_fs fixture @@ -179,9 +203,10 @@ def test_metadata_resolver_local_with_name_and_default_manifest(mock_registry_re metadata_resolver = MetadataResolver(mock_docker_api, mock_registry_resolver) with pytest.raises(FileNotFoundError): - metadata = metadata_resolver.from_local('image', use_local_manifest=False, name='test_manifest', use_edit=True) + metadata_resolver.from_local('image', + use_local_manifest=False, + name='test_manifest', + use_edit=True) mock_open.assert_called_with(DEFAULT_MANIFEST_FILE, 'r') mock_json_loads.assert_not_called() # Ensure json.loads is not called - - diff --git a/tests/sonic_package_manager/test_service_creator.py b/tests/sonic_package_manager/test_service_creator.py index d0d2050d42..8e6edcd0f0 100644 --- a/tests/sonic_package_manager/test_service_creator.py +++ b/tests/sonic_package_manager/test_service_creator.py @@ -43,7 +43,6 @@ def manifest(): }, 'container': { 'privileged': True, - 'entrypoint': '', 'volumes': [ '/etc/sonic:/etc/sonic:ro' ] @@ -603,39 +602,3 @@ def test_rate_limit_update(self, mock_sonic_db, manifest): ], any_order = True ) - - -def test_generate_container_mgmt_entrypoint(): - # Mock Package object with necessary attributes - package_mock = Mock() - package_mock.image_id = "some_image_id" - package_mock.manifest = { - 'service': {'name': 'test'}, - 'container': { - 'privileged': True, - 'entrypoint': '/custom_entrypoint', - 'volumes': [], - 'mounts': [], - 'tmpfs': [], - 'environment': {} - } - } - - mock_feature_registry = Mock() - mock_sonic_db = Mock() - mock_cli_gen = Mock() - mock_config_mgmt = Mock() - - # Mock constants - with patch('sonic_package_manager.service_creator.creator.DOCKER_CTL_SCRIPT_LOCATION', '/usr/bin/'), \ - patch('sonic_package_manager.service_creator.creator.DOCKER_CTL_SCRIPT_TEMPLATE', 'docker_ctl_script_template.tmpl'), \ - patch('sonic_package_manager.service_creator.creator.get_tmpl_path', return_value='mocked_template_path'), \ - patch('sonic_package_manager.service_creator.creator.render_template') as mock_render_template, \ - patch('sonic_package_manager.service_creator.creator.log.info') as mock_log_info: - - srv_instance = ServiceCreator(mock_feature_registry, mock_sonic_db, mock_cli_gen, mock_config_mgmt) - srv_instance.generate_container_mgmt(package_mock) - - # Assertions - mock_render_template.assert_called_once() - mock_log_info.assert_called_once_with('generated /usr/bin/test.sh') diff --git a/tests/test_sonic_installer.py b/tests/test_sonic_installer.py index 648afa5030..66eb972fdf 100644 --- a/tests/test_sonic_installer.py +++ b/tests/test_sonic_installer.py @@ -87,7 +87,8 @@ def rootfs_path_mock(path): call(["chroot", mounted_image_folder, "/usr/lib/docker/docker.sh", "start"]), call(["cp", "/var/lib/sonic-package-manager/packages.json", f"{mounted_image_folder}/tmp/packages.json"]), call(["mkdir", "-p", "/var/lib/sonic-package-manager/manifests"]), - call(["cp", "-arf", "/var/lib/sonic-package-manager/manifests", f"{mounted_image_folder}/var/lib/sonic-package-manager"]), + call(["cp", "-arf", "/var/lib/sonic-package-manager/manifests", + f"{mounted_image_folder}/var/lib/sonic-package-manager"]), call(["touch", f"{mounted_image_folder}/tmp/docker.sock"]), call(["mount", "--bind", "/var/run/docker.sock", f"{mounted_image_folder}/tmp/docker.sock"]), call(["cp", f"{mounted_image_folder}/etc/resolv.conf", "/tmp/resolv.conf.backup"]),