Thrid Party Container Management using Sonic Package Manager - followup2

Author: sg893052

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:

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
 

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" """

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:")

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