Merge ~jugmac00/lpci:fix-run-clean into lpci:main
- Git
- lp:~jugmac00/lpci
- fix-run-clean
- Merge into main
Proposed by
Jürgen Gmach
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 96275f5072e76f857f692ae5ae62931bad293248 | ||||
Proposed branch: | ~jugmac00/lpci:fix-run-clean | ||||
Merge into: | lpci:main | ||||
Diff against target: |
248 lines (+101/-105) 2 files modified
lpcraft/commands/run.py (+54/-57) lpcraft/commands/tests/test_run.py (+47/-48) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Guruprasad | Approve | ||
Review via email: mp+428857@code.launchpad.net |
Commit message
Fix `lpcraft run --clean`
Description of the change
To post a comment you must log in.
Revision history for this message
Guruprasad (lgp171188) : | # |
Revision history for this message
Guruprasad (lgp171188) : | # |
review:
Approve
Revision history for this message
Jürgen Gmach (jugmac00) wrote : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py | |||
2 | index 8012ad4..3c31054 100644 | |||
3 | --- a/lpcraft/commands/run.py | |||
4 | +++ b/lpcraft/commands/run.py | |||
5 | @@ -634,69 +634,66 @@ class RunCommand(BaseCommand): | |||
6 | 634 | 634 | ||
7 | 635 | provider = get_provider() | 635 | provider = get_provider() |
8 | 636 | provider.ensure_provider_is_available() | 636 | provider.ensure_provider_is_available() |
9 | 637 | launched_instances = [] | ||
10 | 638 | 637 | ||
11 | 639 | secrets = {} | 638 | secrets = {} |
12 | 640 | if args.secrets_file: | 639 | if args.secrets_file: |
13 | 641 | with open(args.secrets_file) as f: | 640 | with open(args.secrets_file) as f: |
14 | 642 | content = f.read() | 641 | content = f.read() |
15 | 643 | secrets = yaml.safe_load(content) | 642 | secrets = yaml.safe_load(content) |
72 | 644 | 643 | for stage in config.pipeline: | |
73 | 645 | try: | 644 | stage_failed = False |
74 | 646 | for stage in config.pipeline: | 645 | for job_name in stage: |
75 | 647 | stage_failed = False | 646 | try: |
76 | 648 | for job_name in stage: | 647 | jobs = config.jobs.get(job_name, []) |
77 | 649 | try: | 648 | if not jobs: |
78 | 650 | jobs = config.jobs.get(job_name, []) | 649 | raise CommandError( |
79 | 651 | if not jobs: | 650 | f"No job definition for {job_name!r}" |
80 | 652 | raise CommandError( | 651 | ) |
81 | 653 | f"No job definition for {job_name!r}" | 652 | for job_index, job in enumerate(jobs): |
82 | 654 | ) | 653 | # we prefer package repositories via CLI more |
83 | 655 | for job_index, job in enumerate(jobs): | 654 | # so they need to come first |
84 | 656 | launched_instances.append( | 655 | # also see sources.list(5) |
85 | 657 | _get_job_instance_name(provider, job) | 656 | package_repositories = args.package_repositories |
86 | 658 | ) | 657 | for group in job.package_repositories: |
87 | 659 | # we prefer package repositories via CLI more | 658 | for repository in group.sources_list_lines(): |
88 | 660 | # so they need to come first | 659 | package_repositories.append(repository) |
89 | 661 | # also see sources.list(5) | 660 | _run_job( |
90 | 662 | package_repositories = args.package_repositories | 661 | config, |
91 | 663 | for group in job.package_repositories: | 662 | job_name, |
92 | 664 | for repository in group.sources_list_lines(): | 663 | job_index, |
93 | 665 | package_repositories.append(repository) | 664 | provider, |
94 | 666 | _run_job( | 665 | args.output_directory, |
95 | 667 | config, | 666 | replace_package_repositories=( |
96 | 668 | job_name, | 667 | args.apt_replace_repositories |
97 | 669 | job_index, | 668 | + args.replace_package_repositories |
98 | 670 | provider, | 669 | ), |
99 | 671 | args.output_directory, | 670 | package_repositories=package_repositories, |
100 | 672 | replace_package_repositories=( | 671 | env_from_cli=args.set_env, |
101 | 673 | args.apt_replace_repositories | 672 | plugin_settings=args.plugin_setting, |
102 | 674 | + args.replace_package_repositories | 673 | secrets=secrets, |
103 | 675 | ), | 674 | ) |
104 | 676 | package_repositories=package_repositories, | 675 | |
105 | 677 | env_from_cli=args.set_env, | 676 | except CommandError as e: |
106 | 678 | plugin_settings=args.plugin_setting, | 677 | if len(stage) == 1: |
107 | 679 | secrets=secrets, | 678 | # Single-job stage, so just reraise this |
108 | 680 | ) | 679 | # in order to get simpler error messages. |
109 | 681 | except CommandError as e: | 680 | raise |
110 | 682 | if len(stage) == 1: | 681 | else: |
111 | 683 | # Single-job stage, so just reraise this | 682 | emit.error(e) |
112 | 684 | # in order to get simpler error messages. | 683 | stage_failed = True |
113 | 685 | raise | 684 | finally: |
114 | 686 | else: | 685 | if args.clean: |
115 | 687 | emit.error(e) | 686 | cwd = Path.cwd() |
116 | 688 | stage_failed = True | 687 | provider.clean_project_environments( |
117 | 689 | if stage_failed: | 688 | project_name=cwd.name, |
118 | 690 | raise CommandError( | 689 | project_path=cwd, |
119 | 691 | f"Some jobs in {stage} failed; stopping.", retcode=1 | 690 | instances=[ |
120 | 692 | ) | 691 | _get_job_instance_name(provider, job), |
121 | 693 | finally: | 692 | ], |
122 | 694 | if args.clean: | 693 | ) |
123 | 695 | cwd = Path.cwd() | 694 | if stage_failed: |
124 | 696 | provider.clean_project_environments( | 695 | raise CommandError( |
125 | 697 | project_name=cwd.name, | 696 | f"Some jobs in {stage} failed; stopping.", retcode=1 |
70 | 698 | project_path=cwd, | ||
71 | 699 | instances=launched_instances, | ||
126 | 700 | ) | 697 | ) |
127 | 701 | return 0 | 698 | return 0 |
128 | 702 | 699 | ||
129 | diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py | |||
130 | index e06a28e..93c5fcd 100644 | |||
131 | --- a/lpcraft/commands/tests/test_run.py | |||
132 | +++ b/lpcraft/commands/tests/test_run.py | |||
133 | @@ -2113,46 +2113,20 @@ class TestRun(RunBaseTestCase): | |||
134 | 2113 | expected_instance_names = self.get_instance_names( | 2113 | expected_instance_names = self.get_instance_names( |
135 | 2114 | provider, ("focal", "bionic") | 2114 | provider, ("focal", "bionic") |
136 | 2115 | ) | 2115 | ) |
177 | 2116 | mock_clean_project_environments.assert_called_with( | 2116 | self.assertEqual( |
178 | 2117 | project_name=self.tmp_project_path.name, | 2117 | mock_clean_project_environments.call_args_list, |
179 | 2118 | project_path=self.tmp_project_path, | 2118 | [ |
180 | 2119 | instances=expected_instance_names, | 2119 | call( |
181 | 2120 | ) | 2120 | project_name=self.tmp_project_path.name, |
182 | 2121 | 2121 | project_path=self.tmp_project_path, | |
183 | 2122 | @patch("lpcraft.commands.run.get_provider") | 2122 | instances=[expected_instance_names[0]], |
184 | 2123 | @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64") | 2123 | ), |
185 | 2124 | @patch("lpcraft.providers._lxd.LXDProvider.clean_project_environments") | 2124 | call( |
186 | 2125 | def test_clean_flag_cleans_up_even_when_there_are_errors( | 2125 | project_name=self.tmp_project_path.name, |
187 | 2126 | self, | 2126 | project_path=self.tmp_project_path, |
188 | 2127 | mock_clean_project_environments, | 2127 | instances=[expected_instance_names[1]], |
189 | 2128 | mock_get_host_architecture, | 2128 | ), |
190 | 2129 | mock_get_provider, | 2129 | ], |
151 | 2130 | ): | ||
152 | 2131 | mock_get_provider.return_value = makeLXDProvider() | ||
153 | 2132 | # There are no jobs defined. So there will be an error. | ||
154 | 2133 | config = dedent( | ||
155 | 2134 | """ | ||
156 | 2135 | pipeline: | ||
157 | 2136 | - test | ||
158 | 2137 | |||
159 | 2138 | jobs: {} | ||
160 | 2139 | """ | ||
161 | 2140 | ) | ||
162 | 2141 | Path(".launchpad.yaml").write_text(config) | ||
163 | 2142 | |||
164 | 2143 | result = self.run_command("run", "--clean") | ||
165 | 2144 | |||
166 | 2145 | self.assertThat( | ||
167 | 2146 | result, | ||
168 | 2147 | MatchesStructure.byEquality( | ||
169 | 2148 | exit_code=1, | ||
170 | 2149 | errors=[CommandError("No job definition for 'test'")], | ||
171 | 2150 | ), | ||
172 | 2151 | ) | ||
173 | 2152 | mock_clean_project_environments.assert_called_with( | ||
174 | 2153 | project_name=self.tmp_project_path.name, | ||
175 | 2154 | project_path=self.tmp_project_path, | ||
176 | 2155 | instances=[], | ||
191 | 2156 | ) | 2130 | ) |
192 | 2157 | 2131 | ||
193 | 2158 | @patch("lpcraft.commands.run._run_job") | 2132 | @patch("lpcraft.commands.run._run_job") |
194 | @@ -2216,10 +2190,25 @@ class TestRun(RunBaseTestCase): | |||
195 | 2216 | expected_instance_names = self.get_instance_names( | 2190 | expected_instance_names = self.get_instance_names( |
196 | 2217 | provider, ("focal", "bionic", "jammy") | 2191 | provider, ("focal", "bionic", "jammy") |
197 | 2218 | ) | 2192 | ) |
202 | 2219 | mock_clean_project_environments.assert_called_with( | 2193 | self.assertEqual( |
203 | 2220 | project_name=self.tmp_project_path.name, | 2194 | mock_clean_project_environments.call_args_list, |
204 | 2221 | project_path=self.tmp_project_path, | 2195 | [ |
205 | 2222 | instances=expected_instance_names, | 2196 | call( |
206 | 2197 | project_name=self.tmp_project_path.name, | ||
207 | 2198 | project_path=self.tmp_project_path, | ||
208 | 2199 | instances=[expected_instance_names[0]], | ||
209 | 2200 | ), | ||
210 | 2201 | call( | ||
211 | 2202 | project_name=self.tmp_project_path.name, | ||
212 | 2203 | project_path=self.tmp_project_path, | ||
213 | 2204 | instances=[expected_instance_names[1]], | ||
214 | 2205 | ), | ||
215 | 2206 | call( | ||
216 | 2207 | project_name=self.tmp_project_path.name, | ||
217 | 2208 | project_path=self.tmp_project_path, | ||
218 | 2209 | instances=[expected_instance_names[2]], | ||
219 | 2210 | ), | ||
220 | 2211 | ], | ||
221 | 2223 | ) | 2212 | ) |
222 | 2224 | 2213 | ||
223 | 2225 | @patch("lpcraft.commands.run._run_job") | 2214 | @patch("lpcraft.commands.run._run_job") |
224 | @@ -2269,10 +2258,20 @@ class TestRun(RunBaseTestCase): | |||
225 | 2269 | provider, | 2258 | provider, |
226 | 2270 | ("focal", "bionic"), | 2259 | ("focal", "bionic"), |
227 | 2271 | ) | 2260 | ) |
232 | 2272 | mock_clean_project_environments.assert_called_with( | 2261 | self.assertEqual( |
233 | 2273 | project_name=self.tmp_project_path.name, | 2262 | mock_clean_project_environments.call_args_list, |
234 | 2274 | project_path=self.tmp_project_path, | 2263 | [ |
235 | 2275 | instances=expected_instance_names, | 2264 | call( |
236 | 2265 | project_name=self.tmp_project_path.name, | ||
237 | 2266 | project_path=self.tmp_project_path, | ||
238 | 2267 | instances=[expected_instance_names[0]], | ||
239 | 2268 | ), | ||
240 | 2269 | call( | ||
241 | 2270 | project_name=self.tmp_project_path.name, | ||
242 | 2271 | project_path=self.tmp_project_path, | ||
243 | 2272 | instances=[expected_instance_names[1]], | ||
244 | 2273 | ), | ||
245 | 2274 | ], | ||
246 | 2276 | ) | 2275 | ) |
247 | 2277 | 2276 | ||
248 | 2278 | @patch("lpcraft.commands.run.get_provider") | 2277 | @patch("lpcraft.commands.run.get_provider") |
Thanks for the review!