launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+428857@xxxxxxxxxxxxxxxxxx>]

Jürgen Gmach has proposed merging ~jugmac00/lpcraft:fix-run-clean into lpcraft:main.

Commit message:
Fix `lpcraft run --clean`

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1986374 in lpcraft: "`lpcraft run --clean`  broken"
  https://bugs.launchpad.net/lpcraft/+bug/1986374

For more details, see:
https://code.launchpad.net/~jugmac00/lpcraft/+git/lpcraft/+merge/428857
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/lpcraft:fix-run-clean into lpcraft:main.

diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index 8012ad4..831dc0e 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -641,62 +641,63 @@ class RunCommand(BaseCommand):
             with open(args.secrets_file) as f:
                 content = f.read()
             secrets = yaml.safe_load(content)
-
-        try:
-            for stage in config.pipeline:
-                stage_failed = False
-                for job_name in stage:
-                    try:
-                        jobs = config.jobs.get(job_name, [])
-                        if not jobs:
-                            raise CommandError(
-                                f"No job definition for {job_name!r}"
-                            )
-                        for job_index, job in enumerate(jobs):
-                            launched_instances.append(
-                                _get_job_instance_name(provider, job)
-                            )
-                            # we prefer package repositories via CLI more
-                            # so they need to come first
-                            # also see sources.list(5)
-                            package_repositories = args.package_repositories
-                            for group in job.package_repositories:
-                                for repository in group.sources_list_lines():
-                                    package_repositories.append(repository)
-                            _run_job(
-                                config,
-                                job_name,
-                                job_index,
-                                provider,
-                                args.output_directory,
-                                replace_package_repositories=(
-                                    args.apt_replace_repositories
-                                    + args.replace_package_repositories
-                                ),
-                                package_repositories=package_repositories,
-                                env_from_cli=args.set_env,
-                                plugin_settings=args.plugin_setting,
-                                secrets=secrets,
-                            )
-                    except CommandError as e:
-                        if len(stage) == 1:
-                            # Single-job stage, so just reraise this
-                            # in order to get simpler error messages.
-                            raise
-                        else:
-                            emit.error(e)
-                            stage_failed = True
-                if stage_failed:
-                    raise CommandError(
-                        f"Some jobs in {stage} failed; stopping.", retcode=1
-                    )
-        finally:
-            if args.clean:
-                cwd = Path.cwd()
-                provider.clean_project_environments(
-                    project_name=cwd.name,
-                    project_path=cwd,
-                    instances=launched_instances,
+        for stage in config.pipeline:
+            stage_failed = False
+            for job_name in stage:
+                try:
+                    jobs = config.jobs.get(job_name, [])
+                    if not jobs:
+                        raise CommandError(
+                            f"No job definition for {job_name!r}"
+                        )
+                    for job_index, job in enumerate(jobs):
+                        launched_instances.append(
+                            _get_job_instance_name(provider, job)
+                        )
+                        # we prefer package repositories via CLI more
+                        # so they need to come first
+                        # also see sources.list(5)
+                        package_repositories = args.package_repositories
+                        for group in job.package_repositories:
+                            for repository in group.sources_list_lines():
+                                package_repositories.append(repository)
+                        _run_job(
+                            config,
+                            job_name,
+                            job_index,
+                            provider,
+                            args.output_directory,
+                            replace_package_repositories=(
+                                args.apt_replace_repositories
+                                + args.replace_package_repositories
+                            ),
+                            package_repositories=package_repositories,
+                            env_from_cli=args.set_env,
+                            plugin_settings=args.plugin_setting,
+                            secrets=secrets,
+                        )
+
+                except CommandError as e:
+                    if len(stage) == 1:
+                        # Single-job stage, so just reraise this
+                        # in order to get simpler error messages.
+                        raise
+                    else:
+                        emit.error(e)
+                        stage_failed = True
+                finally:
+                    if args.clean:
+                        cwd = Path.cwd()
+                        provider.clean_project_environments(
+                            project_name=cwd.name,
+                            project_path=cwd,
+                            instances=[
+                                _get_job_instance_name(provider, job),
+                            ],
+                        )
+            if stage_failed:
+                raise CommandError(
+                    f"Some jobs in {stage} failed; stopping.", retcode=1
                 )
         return 0
 
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index e06a28e..93c5fcd 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -2113,46 +2113,20 @@ class TestRun(RunBaseTestCase):
         expected_instance_names = self.get_instance_names(
             provider, ("focal", "bionic")
         )
-        mock_clean_project_environments.assert_called_with(
-            project_name=self.tmp_project_path.name,
-            project_path=self.tmp_project_path,
-            instances=expected_instance_names,
-        )
-
-    @patch("lpcraft.commands.run.get_provider")
-    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
-    @patch("lpcraft.providers._lxd.LXDProvider.clean_project_environments")
-    def test_clean_flag_cleans_up_even_when_there_are_errors(
-        self,
-        mock_clean_project_environments,
-        mock_get_host_architecture,
-        mock_get_provider,
-    ):
-        mock_get_provider.return_value = makeLXDProvider()
-        # There are no jobs defined. So there will be an error.
-        config = dedent(
-            """
-            pipeline:
-                - test
-
-            jobs: {}
-            """
-        )
-        Path(".launchpad.yaml").write_text(config)
-
-        result = self.run_command("run", "--clean")
-
-        self.assertThat(
-            result,
-            MatchesStructure.byEquality(
-                exit_code=1,
-                errors=[CommandError("No job definition for 'test'")],
-            ),
-        )
-        mock_clean_project_environments.assert_called_with(
-            project_name=self.tmp_project_path.name,
-            project_path=self.tmp_project_path,
-            instances=[],
+        self.assertEqual(
+            mock_clean_project_environments.call_args_list,
+            [
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[0]],
+                ),
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[1]],
+                ),
+            ],
         )
 
     @patch("lpcraft.commands.run._run_job")
@@ -2216,10 +2190,25 @@ class TestRun(RunBaseTestCase):
         expected_instance_names = self.get_instance_names(
             provider, ("focal", "bionic", "jammy")
         )
-        mock_clean_project_environments.assert_called_with(
-            project_name=self.tmp_project_path.name,
-            project_path=self.tmp_project_path,
-            instances=expected_instance_names,
+        self.assertEqual(
+            mock_clean_project_environments.call_args_list,
+            [
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[0]],
+                ),
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[1]],
+                ),
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[2]],
+                ),
+            ],
         )
 
     @patch("lpcraft.commands.run._run_job")
@@ -2269,10 +2258,20 @@ class TestRun(RunBaseTestCase):
             provider,
             ("focal", "bionic"),
         )
-        mock_clean_project_environments.assert_called_with(
-            project_name=self.tmp_project_path.name,
-            project_path=self.tmp_project_path,
-            instances=expected_instance_names,
+        self.assertEqual(
+            mock_clean_project_environments.call_args_list,
+            [
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[0]],
+                ),
+                call(
+                    project_name=self.tmp_project_path.name,
+                    project_path=self.tmp_project_path,
+                    instances=[expected_instance_names[1]],
+                ),
+            ],
         )
 
     @patch("lpcraft.commands.run.get_provider")