Merge ~cjwatson/launchpad:fix-snap-charm-push-failure into launchpad:master
- Git
- lp:~cjwatson/launchpad
- fix-snap-charm-push-failure
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | 18f916b3223ff9bb5ebaf503787c0167a7228c1d |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:fix-snap-charm-push-failure |
Merge into: | launchpad:master |
Diff against target: |
1539 lines (+478/-248) 12 files modified
lib/lp/charms/interfaces/charmhubclient.py (+12/-2) lib/lp/charms/interfaces/charmrecipebuildjob.py (+8/-2) lib/lp/charms/model/charmhubclient.py (+4/-6) lib/lp/charms/model/charmrecipebuildjob.py (+27/-6) lib/lp/charms/tests/test_charmhubclient.py (+40/-28) lib/lp/charms/tests/test_charmrecipebuildjob.py (+131/-48) lib/lp/snappy/interfaces/snapbuildjob.py (+10/-4) lib/lp/snappy/interfaces/snapstoreclient.py (+12/-2) lib/lp/snappy/model/snapbuildjob.py (+35/-13) lib/lp/snappy/model/snapstoreclient.py (+8/-15) lib/lp/snappy/tests/test_snapbuildjob.py (+145/-51) lib/lp/snappy/tests/test_snapstoreclient.py (+46/-71) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Cristian Gonzalez (community) | Approve | ||
Review via email: mp+408582@code.launchpad.net |
Commit message
Fix handling of snap/charm push failures after upload
Description of the change
`SnapBuildJob` and `CharmRecipeBui
Resolve this by splitting the "upload" step into "upload file" and "push", which are retried separately.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/charms/interfaces/charmhubclient.py b/lib/lp/charms/interfaces/charmhubclient.py |
2 | index 7f6443e..9a7cc0f 100644 |
3 | --- a/lib/lp/charms/interfaces/charmhubclient.py |
4 | +++ b/lib/lp/charms/interfaces/charmhubclient.py |
5 | @@ -94,10 +94,20 @@ class ICharmhubClient(Interface): |
6 | Candid caveat. |
7 | """ |
8 | |
9 | - def upload(build): |
10 | - """Upload a charm recipe build to CharmHub. |
11 | + def uploadFile(lfa): |
12 | + """Upload a file to Charmhub. |
13 | + |
14 | + :param lfa: The `ILibraryFileAlias` to upload. |
15 | + :return: An upload ID. |
16 | + :raises UploadFailedResponse: if uploading the file to Charmhub |
17 | + failed. |
18 | + """ |
19 | + |
20 | + def push(build, upload_id): |
21 | + """Push a charm recipe build to CharmHub. |
22 | |
23 | :param build: The `ICharmRecipeBuild` to upload. |
24 | + :param upload_id: An upload ID previously returned by `uploadFile`. |
25 | :return: A URL to poll for upload processing status. |
26 | :raises UnauthorizedUploadResponse: if the user who authorised this |
27 | upload is not themselves authorised to upload the snap in |
28 | diff --git a/lib/lp/charms/interfaces/charmrecipebuildjob.py b/lib/lp/charms/interfaces/charmrecipebuildjob.py |
29 | index b0d1e20..2ae320d 100644 |
30 | --- a/lib/lp/charms/interfaces/charmrecipebuildjob.py |
31 | +++ b/lib/lp/charms/interfaces/charmrecipebuildjob.py |
32 | @@ -55,14 +55,20 @@ class ICharmhubUploadJob(IRunnableJob): |
33 | error_detail = TextLine( |
34 | title=_("Error message detail"), required=False, readonly=True) |
35 | |
36 | - store_revision = Int( |
37 | - title=_("The revision assigned to this build by Charmhub"), |
38 | + upload_id = Int( |
39 | + title=_( |
40 | + "The ID returned by Charmhub when uploading this build's charm " |
41 | + "file."), |
42 | required=False, readonly=True) |
43 | |
44 | status_url = TextLine( |
45 | title=_("The URL on Charmhub to get the status of this build"), |
46 | required=False, readonly=True) |
47 | |
48 | + store_revision = Int( |
49 | + title=_("The revision assigned to this build by Charmhub"), |
50 | + required=False, readonly=True) |
51 | + |
52 | |
53 | class ICharmhubUploadJobSource(IJobSource): |
54 | |
55 | diff --git a/lib/lp/charms/model/charmhubclient.py b/lib/lp/charms/model/charmhubclient.py |
56 | index 4b275b0..33ea281 100644 |
57 | --- a/lib/lp/charms/model/charmhubclient.py |
58 | +++ b/lib/lp/charms/model/charmhubclient.py |
59 | @@ -147,7 +147,7 @@ class CharmhubClient: |
60 | timeline_action.finish() |
61 | |
62 | @classmethod |
63 | - def _uploadToStorage(cls, lfa): |
64 | + def uploadFile(cls, lfa): |
65 | """Upload a single file to Charmhub's storage.""" |
66 | assert config.charms.charmhub_storage_url is not None |
67 | unscanned_upload_url = urlappend( |
68 | @@ -182,12 +182,10 @@ class CharmhubClient: |
69 | lfa.close() |
70 | |
71 | @classmethod |
72 | - def _push(cls, build, upload_id): |
73 | + def push(cls, build, upload_id): |
74 | """Push an already-uploaded charm to Charmhub.""" |
75 | recipe = build.recipe |
76 | - assert config.charms.charmhub_url is not None |
77 | - assert recipe.store_name is not None |
78 | - assert recipe.store_secrets is not None |
79 | + assert recipe.can_upload_to_store |
80 | push_url = urlappend( |
81 | config.charms.charmhub_url, |
82 | "v1/charm/{}/revisions".format(quote(recipe.store_name))) |
83 | @@ -218,7 +216,7 @@ class CharmhubClient: |
84 | for _, lfa, _ in build.getFiles(): |
85 | if not lfa.filename.endswith(".charm"): |
86 | continue |
87 | - upload_id = cls._uploadToStorage(lfa) |
88 | + upload_id = cls.uploadFile(lfa) |
89 | return cls._push(build, upload_id) |
90 | |
91 | @classmethod |
92 | diff --git a/lib/lp/charms/model/charmrecipebuildjob.py b/lib/lp/charms/model/charmrecipebuildjob.py |
93 | index bafe2ba..cfe86a6 100644 |
94 | --- a/lib/lp/charms/model/charmrecipebuildjob.py |
95 | +++ b/lib/lp/charms/model/charmrecipebuildjob.py |
96 | @@ -230,16 +230,16 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived): |
97 | self.metadata["error_detail"] = detail |
98 | |
99 | @property |
100 | - def store_revision(self): |
101 | + def upload_id(self): |
102 | """See `ICharmhubUploadJob`.""" |
103 | - return self.store_metadata.get("store_revision") |
104 | + return self.store_metadata.get("upload_id") |
105 | |
106 | - @store_revision.setter |
107 | - def store_revision(self, revision): |
108 | + @upload_id.setter |
109 | + def upload_id(self, upload_id): |
110 | """See `ICharmhubUploadJob`.""" |
111 | if self.build.store_upload_metadata is None: |
112 | self.build.store_upload_metadata = {} |
113 | - self.build.store_upload_metadata["store_revision"] = revision |
114 | + self.build.store_upload_metadata["upload_id"] = upload_id |
115 | |
116 | @property |
117 | def status_url(self): |
118 | @@ -252,6 +252,18 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived): |
119 | self.build.store_upload_metadata = {} |
120 | self.build.store_upload_metadata["status_url"] = url |
121 | |
122 | + @property |
123 | + def store_revision(self): |
124 | + """See `ICharmhubUploadJob`.""" |
125 | + return self.store_metadata.get("store_revision") |
126 | + |
127 | + @store_revision.setter |
128 | + def store_revision(self, revision): |
129 | + """See `ICharmhubUploadJob`.""" |
130 | + if self.build.store_upload_metadata is None: |
131 | + self.build.store_upload_metadata = {} |
132 | + self.build.store_upload_metadata["store_revision"] = revision |
133 | + |
134 | # Ideally we'd just override Job._set_status or similar, but |
135 | # lazr.delegates makes that difficult, so we use this to override all |
136 | # the individual Job lifecycle methods instead. |
137 | @@ -311,8 +323,17 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived): |
138 | client = getUtility(ICharmhubClient) |
139 | try: |
140 | try: |
141 | + lfa = next((row[1] for row in self.build.getFiles()), None) |
142 | + if lfa is None: |
143 | + # Nothing to do. |
144 | + self.error_message = None |
145 | + return |
146 | + if "upload_id" not in self.store_metadata: |
147 | + self.upload_id = client.uploadFile(lfa) |
148 | + # We made progress, so reset attempt_count. |
149 | + self.attempt_count = 1 |
150 | if "status_url" not in self.store_metadata: |
151 | - self.status_url = client.upload(self.build) |
152 | + self.status_url = client.push(self.build, self.upload_id) |
153 | # We made progress, so reset attempt_count. |
154 | self.attempt_count = 1 |
155 | if self.store_revision is None: |
156 | diff --git a/lib/lp/charms/tests/test_charmhubclient.py b/lib/lp/charms/tests/test_charmhubclient.py |
157 | index 87e9cca..018709d 100644 |
158 | --- a/lib/lp/charms/tests/test_charmhubclient.py |
159 | +++ b/lib/lp/charms/tests/test_charmhubclient.py |
160 | @@ -302,18 +302,15 @@ class TestCharmhubClient(TestCaseWithFactory): |
161 | return build |
162 | |
163 | @responses.activate |
164 | - def test_upload(self): |
165 | - self._setUpSecretStorage() |
166 | - build = self.makeUploadableCharmRecipeBuild() |
167 | + def test_uploadFile(self): |
168 | + charm_lfa = self.factory.makeLibraryFileAlias( |
169 | + filename="test-charm.charm", content="dummy charm content") |
170 | transaction.commit() |
171 | self._addUnscannedUploadResponse() |
172 | - self._addCharmPushResponse("test-charm") |
173 | # XXX cjwatson 2021-08-19: Use |
174 | # config.ICharmhubUploadJobSource.dbuser once that job exists. |
175 | with dbuser("charm-build-job"): |
176 | - self.assertEqual( |
177 | - "/v1/charm/test-charm/revisions/review?upload-id=123", |
178 | - self.client.upload(build)) |
179 | + self.assertEqual(1, self.client.uploadFile(charm_lfa)) |
180 | requests = [call.request for call in responses.calls] |
181 | self.assertThat(requests, MatchesListwise([ |
182 | RequestMatches( |
183 | @@ -326,6 +323,40 @@ class TestCharmhubClient(TestCaseWithFactory): |
184 | value="dummy charm content", |
185 | content_type="application/octet-stream", |
186 | )}), |
187 | + ])) |
188 | + |
189 | + @responses.activate |
190 | + def test_uploadFile_error(self): |
191 | + charm_lfa = self.factory.makeLibraryFileAlias( |
192 | + filename="test-charm.charm", content="dummy charm content") |
193 | + transaction.commit() |
194 | + responses.add( |
195 | + "POST", "http://storage.charmhub.example/unscanned-upload/", |
196 | + status=502, body="The proxy exploded.\n") |
197 | + # XXX cjwatson 2021-08-19: Use |
198 | + # config.ICharmhubUploadJobSource.dbuser once that job exists. |
199 | + with dbuser("charm-build-job"): |
200 | + err = self.assertRaises( |
201 | + UploadFailedResponse, self.client.uploadFile, charm_lfa) |
202 | + self.assertEqual("502 Server Error: Bad Gateway", str(err)) |
203 | + self.assertThat(err, MatchesStructure( |
204 | + detail=Equals("The proxy exploded.\n"), |
205 | + can_retry=Is(True))) |
206 | + |
207 | + @responses.activate |
208 | + def test_push(self): |
209 | + self._setUpSecretStorage() |
210 | + build = self.makeUploadableCharmRecipeBuild() |
211 | + transaction.commit() |
212 | + self._addCharmPushResponse("test-charm") |
213 | + # XXX cjwatson 2021-08-19: Use |
214 | + # config.ICharmhubUploadJobSource.dbuser once that job exists. |
215 | + with dbuser("charm-build-job"): |
216 | + self.assertEqual( |
217 | + "/v1/charm/test-charm/revisions/review?upload-id=123", |
218 | + self.client.push(build, 1)) |
219 | + requests = [call.request for call in responses.calls] |
220 | + self.assertThat(requests, MatchesListwise([ |
221 | RequestMatches( |
222 | url=Equals( |
223 | "http://charmhub.example/v1/charm/test-charm/revisions"), |
224 | @@ -337,11 +368,10 @@ class TestCharmhubClient(TestCaseWithFactory): |
225 | ])) |
226 | |
227 | @responses.activate |
228 | - def test_upload_unauthorized(self): |
229 | + def test_push_unauthorized(self): |
230 | self._setUpSecretStorage() |
231 | build = self.makeUploadableCharmRecipeBuild() |
232 | transaction.commit() |
233 | - self._addUnscannedUploadResponse() |
234 | charm_push_error = { |
235 | "code": "permission-required", |
236 | "message": "Missing required permission: package-manage-revisions", |
237 | @@ -356,25 +386,7 @@ class TestCharmhubClient(TestCaseWithFactory): |
238 | self.assertRaisesWithContent( |
239 | UnauthorizedUploadResponse, |
240 | "Missing required permission: package-manage-revisions", |
241 | - self.client.upload, build) |
242 | - |
243 | - @responses.activate |
244 | - def test_upload_file_error(self): |
245 | - self._setUpSecretStorage() |
246 | - build = self.makeUploadableCharmRecipeBuild() |
247 | - transaction.commit() |
248 | - responses.add( |
249 | - "POST", "http://storage.charmhub.example/unscanned-upload/", |
250 | - status=502, body="The proxy exploded.\n") |
251 | - # XXX cjwatson 2021-08-19: Use |
252 | - # config.ICharmhubUploadJobSource.dbuser once that job exists. |
253 | - with dbuser("charm-build-job"): |
254 | - err = self.assertRaises( |
255 | - UploadFailedResponse, self.client.upload, build) |
256 | - self.assertEqual("502 Server Error: Bad Gateway", str(err)) |
257 | - self.assertThat(err, MatchesStructure( |
258 | - detail=Equals("The proxy exploded.\n"), |
259 | - can_retry=Is(True))) |
260 | + self.client.push, build, 1) |
261 | |
262 | @responses.activate |
263 | def test_checkStatus_pending(self): |
264 | diff --git a/lib/lp/charms/tests/test_charmrecipebuildjob.py b/lib/lp/charms/tests/test_charmrecipebuildjob.py |
265 | index 9bc020d..d8c1e86 100644 |
266 | --- a/lib/lp/charms/tests/test_charmrecipebuildjob.py |
267 | +++ b/lib/lp/charms/tests/test_charmrecipebuildjob.py |
268 | @@ -78,7 +78,8 @@ def run_isolated_jobs(jobs): |
269 | class FakeCharmhubClient: |
270 | |
271 | def __init__(self): |
272 | - self.upload = FakeMethod() |
273 | + self.uploadFile = FakeMethod() |
274 | + self.push = FakeMethod() |
275 | self.checkStatus = FakeMethod() |
276 | self.release = FakeMethod() |
277 | |
278 | @@ -129,10 +130,13 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
279 | repr(job)) |
280 | |
281 | def makeCharmRecipeBuild(self, **kwargs): |
282 | - # Make a build with a builder and a webhook. |
283 | + # Make a build with a builder, a file, and a webhook. |
284 | build = self.factory.makeCharmRecipeBuild( |
285 | builder=self.factory.makeBuilder(), **kwargs) |
286 | build.updateStatus(BuildStatus.FULLYBUILT) |
287 | + charm_lfa = self.factory.makeLibraryFileAlias( |
288 | + filename="test-charm.charm", content="dummy charm content") |
289 | + self.factory.makeCharmFile(build=build, library_file=charm_lfa) |
290 | self.factory.makeWebhook( |
291 | target=build.recipe, event_types=["charm-recipe:build:0.1"]) |
292 | return build |
293 | @@ -176,15 +180,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
294 | # revision. |
295 | logger = self.useFixture(FakeLogger()) |
296 | build = self.makeCharmRecipeBuild() |
297 | + charm_lfa = build.getFiles()[0][1] |
298 | self.assertContentEqual([], build.store_upload_jobs) |
299 | job = CharmhubUploadJob.create(build) |
300 | client = FakeCharmhubClient() |
301 | - client.upload.result = self.status_url |
302 | + client.uploadFile.result = 1 |
303 | + client.push.result = self.status_url |
304 | client.checkStatus.result = 1 |
305 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
306 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
307 | run_isolated_jobs([job]) |
308 | - self.assertEqual([((build,), {})], client.upload.calls) |
309 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
310 | + self.assertEqual([((build, 1), {})], client.push.calls) |
311 | self.assertEqual( |
312 | [((build, self.status_url), {})], client.checkStatus.calls) |
313 | self.assertEqual([], client.release.calls) |
314 | @@ -198,14 +205,16 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
315 | # A failed run sets the store upload status to FAILED. |
316 | logger = self.useFixture(FakeLogger()) |
317 | build = self.makeCharmRecipeBuild() |
318 | + charm_lfa = build.getFiles()[0][1] |
319 | self.assertContentEqual([], build.store_upload_jobs) |
320 | job = CharmhubUploadJob.create(build) |
321 | client = FakeCharmhubClient() |
322 | - client.upload.failure = ValueError("An upload failure") |
323 | + client.uploadFile.failure = ValueError("An upload failure") |
324 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
325 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
326 | run_isolated_jobs([job]) |
327 | - self.assertEqual([((build,), {})], client.upload.calls) |
328 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
329 | + self.assertEqual([], client.push.calls) |
330 | self.assertEqual([], client.checkStatus.calls) |
331 | self.assertEqual([], client.release.calls) |
332 | self.assertContentEqual([job], build.store_upload_jobs) |
333 | @@ -225,15 +234,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
334 | build = self.makeCharmRecipeBuild( |
335 | requester=requester_team, name="test-charm", owner=requester_team, |
336 | project=project) |
337 | + charm_lfa = build.getFiles()[0][1] |
338 | self.assertContentEqual([], build.store_upload_jobs) |
339 | job = CharmhubUploadJob.create(build) |
340 | client = FakeCharmhubClient() |
341 | - client.upload.failure = UnauthorizedUploadResponse( |
342 | + client.uploadFile.result = 1 |
343 | + client.uploadFile.failure = UnauthorizedUploadResponse( |
344 | "Authorization failed.") |
345 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
346 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
347 | run_isolated_jobs([job]) |
348 | - self.assertEqual([((build,), {})], client.upload.calls) |
349 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
350 | + self.assertEqual([], client.push.calls) |
351 | self.assertEqual([], client.checkStatus.calls) |
352 | self.assertEqual([], client.release.calls) |
353 | self.assertContentEqual([job], build.store_upload_jobs) |
354 | @@ -275,15 +287,17 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
355 | # retried. |
356 | logger = self.useFixture(FakeLogger()) |
357 | build = self.makeCharmRecipeBuild() |
358 | + charm_lfa = build.getFiles()[0][1] |
359 | self.assertContentEqual([], build.store_upload_jobs) |
360 | job = CharmhubUploadJob.create(build) |
361 | client = FakeCharmhubClient() |
362 | - client.upload.failure = UploadFailedResponse( |
363 | + client.uploadFile.failure = UploadFailedResponse( |
364 | "Proxy error", can_retry=True) |
365 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
366 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
367 | run_isolated_jobs([job]) |
368 | - self.assertEqual([((build,), {})], client.upload.calls) |
369 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
370 | + self.assertEqual([], client.push.calls) |
371 | self.assertEqual([], client.checkStatus.calls) |
372 | self.assertEqual([], client.release.calls) |
373 | self.assertContentEqual([job], build.store_upload_jobs) |
374 | @@ -295,13 +309,15 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
375 | # Try again. The upload part of the job is retried, and this time |
376 | # it succeeds. |
377 | job.scheduled_start = None |
378 | - client.upload.calls = [] |
379 | - client.upload.failure = None |
380 | - client.upload.result = self.status_url |
381 | + client.uploadFile.calls = [] |
382 | + client.uploadFile.failure = None |
383 | + client.uploadFile.result = 1 |
384 | + client.push.result = self.status_url |
385 | client.checkStatus.result = 1 |
386 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
387 | run_isolated_jobs([job]) |
388 | - self.assertEqual([((build,), {})], client.upload.calls) |
389 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
390 | + self.assertEqual([((build, 1), {})], client.push.calls) |
391 | self.assertEqual( |
392 | [((build, self.status_url), {})], client.checkStatus.calls) |
393 | self.assertEqual([], client.release.calls) |
394 | @@ -323,15 +339,17 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
395 | build = self.makeCharmRecipeBuild( |
396 | requester=requester_team, name="test-charm", owner=requester_team, |
397 | project=project) |
398 | + charm_lfa = build.getFiles()[0][1] |
399 | self.assertContentEqual([], build.store_upload_jobs) |
400 | job = CharmhubUploadJob.create(build) |
401 | client = FakeCharmhubClient() |
402 | - client.upload.failure = UploadFailedResponse( |
403 | + client.uploadFile.failure = UploadFailedResponse( |
404 | "Failed to upload", detail="The proxy exploded.\n") |
405 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
406 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
407 | run_isolated_jobs([job]) |
408 | - self.assertEqual([((build,), {})], client.upload.calls) |
409 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
410 | + self.assertEqual([], client.push.calls) |
411 | self.assertEqual([], client.checkStatus.calls) |
412 | self.assertEqual([], client.release.calls) |
413 | self.assertContentEqual([job], build.store_upload_jobs) |
414 | @@ -374,15 +392,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
415 | # charm schedules itself to be retried. |
416 | logger = self.useFixture(FakeLogger()) |
417 | build = self.makeCharmRecipeBuild() |
418 | + charm_lfa = build.getFiles()[0][1] |
419 | self.assertContentEqual([], build.store_upload_jobs) |
420 | job = CharmhubUploadJob.create(build) |
421 | client = FakeCharmhubClient() |
422 | - client.upload.result = self.status_url |
423 | + client.uploadFile.result = 2 |
424 | + client.push.result = self.status_url |
425 | client.checkStatus.failure = UploadNotReviewedYetResponse() |
426 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
427 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
428 | run_isolated_jobs([job]) |
429 | - self.assertEqual([((build,), {})], client.upload.calls) |
430 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
431 | + self.assertEqual([((build, 2), {})], client.push.calls) |
432 | self.assertEqual( |
433 | [((build, self.status_url), {})], client.checkStatus.calls) |
434 | self.assertEqual([], client.release.calls) |
435 | @@ -392,16 +413,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
436 | self.assertEqual([], pop_notifications()) |
437 | self.assertEqual(JobStatus.WAITING, job.job.status) |
438 | self.assertWebhookDeliveries(build, ["Pending"], logger) |
439 | - # Try again. The upload part of the job is not retried, and this |
440 | - # time the review completes. |
441 | + # Try again. The upload and push parts of the job are not retried, |
442 | + # and this time the review completes. |
443 | job.scheduled_start = None |
444 | - client.upload.calls = [] |
445 | + client.uploadFile.calls = [] |
446 | + client.push.calls = [] |
447 | client.checkStatus.calls = [] |
448 | client.checkStatus.failure = None |
449 | client.checkStatus.result = 1 |
450 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
451 | run_isolated_jobs([job]) |
452 | - self.assertEqual([], client.upload.calls) |
453 | + self.assertEqual([], client.uploadFile.calls) |
454 | + self.assertEqual([], client.push.calls) |
455 | self.assertEqual( |
456 | [((build, self.status_url), {})], client.checkStatus.calls) |
457 | self.assertEqual([], client.release.calls) |
458 | @@ -422,16 +445,19 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
459 | build = self.makeCharmRecipeBuild( |
460 | requester=requester_team, name="test-charm", owner=requester_team, |
461 | project=project) |
462 | + charm_lfa = build.getFiles()[0][1] |
463 | self.assertContentEqual([], build.store_upload_jobs) |
464 | job = CharmhubUploadJob.create(build) |
465 | client = FakeCharmhubClient() |
466 | - client.upload.result = self.status_url |
467 | + client.uploadFile.result = 2 |
468 | + client.push.result = self.status_url |
469 | client.checkStatus.failure = ReviewFailedResponse( |
470 | "Review failed.\nCharm is terrible.") |
471 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
472 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
473 | run_isolated_jobs([job]) |
474 | - self.assertEqual([((build,), {})], client.upload.calls) |
475 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
476 | + self.assertEqual([((build, 2), {})], client.push.calls) |
477 | self.assertEqual( |
478 | [((build, self.status_url), {})], client.checkStatus.calls) |
479 | self.assertEqual([], client.release.calls) |
480 | @@ -472,15 +498,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
481 | # channels does so. |
482 | logger = self.useFixture(FakeLogger()) |
483 | build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"]) |
484 | + charm_lfa = build.getFiles()[0][1] |
485 | self.assertContentEqual([], build.store_upload_jobs) |
486 | job = CharmhubUploadJob.create(build) |
487 | client = FakeCharmhubClient() |
488 | - client.upload.result = self.status_url |
489 | + client.uploadFile.result = 1 |
490 | + client.push.result = self.status_url |
491 | client.checkStatus.result = 1 |
492 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
493 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
494 | run_isolated_jobs([job]) |
495 | - self.assertEqual([((build,), {})], client.upload.calls) |
496 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
497 | + self.assertEqual([((build, 1), {})], client.push.calls) |
498 | self.assertEqual( |
499 | [((build, self.status_url), {})], client.checkStatus.calls) |
500 | self.assertEqual([((build, 1), {})], client.release.calls) |
501 | @@ -501,16 +530,19 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
502 | build = self.makeCharmRecipeBuild( |
503 | requester=requester_team, name="test-charm", owner=requester_team, |
504 | project=project, store_channels=["stable", "edge"]) |
505 | + charm_lfa = build.getFiles()[0][1] |
506 | self.assertContentEqual([], build.store_upload_jobs) |
507 | job = CharmhubUploadJob.create(build) |
508 | client = FakeCharmhubClient() |
509 | - client.upload.result = self.status_url |
510 | + client.uploadFile.result = 1 |
511 | + client.push.result = self.status_url |
512 | client.checkStatus.result = 1 |
513 | client.release.failure = ReleaseFailedResponse("Failed to release") |
514 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
515 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
516 | JobRunner([job]).runAll() |
517 | - self.assertEqual([((build,), {})], client.upload.calls) |
518 | + self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls) |
519 | + self.assertEqual([((build, 1), {})], client.push.calls) |
520 | self.assertEqual( |
521 | [((build, self.status_url), {})], client.checkStatus.calls) |
522 | self.assertEqual([((build, 1), {})], client.release.calls) |
523 | @@ -552,16 +584,26 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
524 | build = self.makeCharmRecipeBuild() |
525 | job = CharmhubUploadJob.create(build) |
526 | client = FakeCharmhubClient() |
527 | - client.upload.failure = UploadFailedResponse( |
528 | + client.uploadFile.failure = UploadFailedResponse( |
529 | "Proxy error", can_retry=True) |
530 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
531 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
532 | run_isolated_jobs([job]) |
533 | - self.assertNotIn("status_url", job.metadata) |
534 | + self.assertNotIn("upload_id", job.store_metadata) |
535 | self.assertEqual(timedelta(seconds=60), job.retry_delay) |
536 | job.scheduled_start = None |
537 | - client.upload.failure = None |
538 | - client.upload.result = self.status_url |
539 | + client.uploadFile.failure = None |
540 | + client.uploadFile.result = 1 |
541 | + client.push.failure = UploadFailedResponse( |
542 | + "Proxy error", can_retry=True) |
543 | + with dbuser(config.ICharmhubUploadJobSource.dbuser): |
544 | + run_isolated_jobs([job]) |
545 | + self.assertIn("upload_id", job.build.store_upload_metadata) |
546 | + self.assertNotIn("status_url", job.store_metadata) |
547 | + self.assertEqual(timedelta(seconds=60), job.retry_delay) |
548 | + job.scheduled_start = None |
549 | + client.push.failure = None |
550 | + client.push.result = self.status_url |
551 | client.checkStatus.failure = UploadNotReviewedYetResponse() |
552 | for expected_delay in (15, 15, 30, 30, 60): |
553 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
554 | @@ -579,39 +621,80 @@ class TestCharmhubUploadJob(TestCaseWithFactory): |
555 | self.assertEqual(JobStatus.COMPLETED, job.job.status) |
556 | |
557 | def test_retry_after_upload_does_not_upload(self): |
558 | - # If the job has uploaded, but failed to release, it should |
559 | - # not attempt to upload again on the next run. |
560 | + # If the job has uploaded but failed to push, it should not attempt |
561 | + # to upload again on the next run. |
562 | self.useFixture(FakeLogger()) |
563 | build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"]) |
564 | self.assertContentEqual([], build.store_upload_jobs) |
565 | job = CharmhubUploadJob.create(build) |
566 | client = FakeCharmhubClient() |
567 | - client.upload.result = self.status_url |
568 | - client.checkStatus.result = 1 |
569 | - client.release.failure = ReleaseFailedResponse( |
570 | + client.uploadFile.result = 1 |
571 | + client.push.failure = UploadFailedResponse( |
572 | "Proxy error", can_retry=True) |
573 | self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
574 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
575 | run_isolated_jobs([job]) |
576 | |
577 | - previous_upload = client.upload.calls |
578 | - previous_checkStatus = client.checkStatus.calls |
579 | - len_previous_release = len(client.release.calls) |
580 | + # We performed the upload as expected, but the store failed to release |
581 | + # it. |
582 | + self.assertIsNone(job.store_revision) |
583 | + self.assertEqual(timedelta(seconds=60), job.retry_delay) |
584 | + self.assertEqual(1, len(client.uploadFile.calls)) |
585 | + self.assertIsNone(job.error_message) |
586 | + |
587 | + # Run the job again. |
588 | + client.uploadFile.calls = [] |
589 | + client.push.calls = [] |
590 | + client.push.failure = None |
591 | + client.push.result = self.status_url |
592 | + client.checkStatus.result = 1 |
593 | + with dbuser(config.ICharmhubUploadJobSource.dbuser): |
594 | + run_isolated_jobs([job]) |
595 | |
596 | - # Check we uploaded as expected |
597 | + # The push has now succeeded. Make sure that we didn't try to |
598 | + # upload the file again first. |
599 | self.assertEqual(1, job.store_revision) |
600 | - self.assertEqual(timedelta(seconds=60), job.retry_delay) |
601 | - self.assertEqual(1, len(client.upload.calls)) |
602 | + self.assertEqual([], client.uploadFile.calls) |
603 | + self.assertEqual(1, len(client.push.calls)) |
604 | self.assertIsNone(job.error_message) |
605 | |
606 | - # Run the job again |
607 | + def test_retry_after_push_does_not_upload_or_push(self): |
608 | + # If the job has uploaded and pushed but has not yet been reviewed, |
609 | + # it should not attempt to upload or push again on the next run. |
610 | + self.useFixture(FakeLogger()) |
611 | + build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"]) |
612 | + self.assertContentEqual([], build.store_upload_jobs) |
613 | + job = CharmhubUploadJob.create(build) |
614 | + client = FakeCharmhubClient() |
615 | + client.uploadFile.result = 1 |
616 | + client.push.result = self.status_url |
617 | + client.checkStatus.failure = UploadNotReviewedYetResponse() |
618 | + self.useFixture(ZopeUtilityFixture(client, ICharmhubClient)) |
619 | with dbuser(config.ICharmhubUploadJobSource.dbuser): |
620 | run_isolated_jobs([job]) |
621 | |
622 | - # We should not have called `upload`, but moved straight to `release` |
623 | - self.assertEqual(previous_upload, client.upload.calls) |
624 | - self.assertEqual(previous_checkStatus, client.checkStatus.calls) |
625 | - self.assertEqual(len_previous_release + 1, len(client.release.calls)) |
626 | + # We performed the upload as expected, but the store is still |
627 | + # reviewing it. |
628 | + self.assertIsNone(job.store_revision) |
629 | + self.assertEqual(timedelta(seconds=15), job.retry_delay) |
630 | + self.assertEqual(1, len(client.uploadFile.calls)) |
631 | + self.assertIsNone(job.error_message) |
632 | + |
633 | + # Run the job again. |
634 | + client.uploadFile.calls = [] |
635 | + client.push.calls = [] |
636 | + client.checkStatus.calls = [] |
637 | + client.checkStatus.failure = None |
638 | + client.checkStatus.result = 1 |
639 | + with dbuser(config.ICharmhubUploadJobSource.dbuser): |
640 | + run_isolated_jobs([job]) |
641 | + |
642 | + # The store has now reviewed the upload. Make sure that we didn't |
643 | + # try to upload or push it again first. |
644 | + self.assertEqual(1, job.store_revision) |
645 | + self.assertEqual([], client.uploadFile.calls) |
646 | + self.assertEqual([], client.push.calls) |
647 | + self.assertEqual(1, len(client.checkStatus.calls)) |
648 | self.assertIsNone(job.error_message) |
649 | |
650 | def test_with_build_metadata_as_none(self): |
651 | diff --git a/lib/lp/snappy/interfaces/snapbuildjob.py b/lib/lp/snappy/interfaces/snapbuildjob.py |
652 | index 3b174f4..826bc07 100644 |
653 | --- a/lib/lp/snappy/interfaces/snapbuildjob.py |
654 | +++ b/lib/lp/snappy/interfaces/snapbuildjob.py |
655 | @@ -61,6 +61,16 @@ class ISnapStoreUploadJob(IRunnableJob): |
656 | error_detail = TextLine( |
657 | title=_("Error message detail"), required=False, readonly=True) |
658 | |
659 | + upload_id = Int( |
660 | + title=_( |
661 | + "The ID returned by the store when uploading this build's snap " |
662 | + "file."), |
663 | + required=False, readonly=True) |
664 | + |
665 | + status_url = TextLine( |
666 | + title=_("The URL on the store to get the status of this build"), |
667 | + required=False, readonly=True) |
668 | + |
669 | store_url = TextLine( |
670 | title=_("The URL on the store corresponding to this build"), |
671 | required=False, readonly=True) |
672 | @@ -69,10 +79,6 @@ class ISnapStoreUploadJob(IRunnableJob): |
673 | title=_("The revision assigned to this build by the store"), |
674 | required=False, readonly=True) |
675 | |
676 | - status_url = TextLine( |
677 | - title=_("The URL on the store to get the status of this build"), |
678 | - required=False, readonly=True) |
679 | - |
680 | |
681 | class ISnapStoreUploadJobSource(IJobSource): |
682 | |
683 | diff --git a/lib/lp/snappy/interfaces/snapstoreclient.py b/lib/lp/snappy/interfaces/snapstoreclient.py |
684 | index e9ff638..f4db218 100644 |
685 | --- a/lib/lp/snappy/interfaces/snapstoreclient.py |
686 | +++ b/lib/lp/snappy/interfaces/snapstoreclient.py |
687 | @@ -88,10 +88,20 @@ class ISnapStoreClient(Interface): |
688 | this snap. |
689 | """ |
690 | |
691 | - def upload(snapbuild): |
692 | - """Upload a snap build to the store. |
693 | + def uploadFile(lfa): |
694 | + """Upload a file to the store. |
695 | + |
696 | + :param lfa: The `ILibraryFileAlias` to upload. |
697 | + :return: An upload ID. |
698 | + :raises UploadFailedResponse: if uploading the file to the store |
699 | + failed. |
700 | + """ |
701 | + |
702 | + def push(snapbuild, upload_id): |
703 | + """Push a snap build to the store. |
704 | |
705 | :param snapbuild: The `ISnapBuild` to upload. |
706 | + :param upload_id: An upload ID previously returned by `uploadFile`. |
707 | :return: A URL to poll for upload processing status. |
708 | :raises BadRefreshResponse: if the authorising macaroons need to be |
709 | refreshed, but attempting to do so fails. |
710 | diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py |
711 | index a8fc49f..8e79973 100644 |
712 | --- a/lib/lp/snappy/model/snapbuildjob.py |
713 | +++ b/lib/lp/snappy/model/snapbuildjob.py |
714 | @@ -1,4 +1,4 @@ |
715 | -# Copyright 2016-2019 Canonical Ltd. This software is licensed under the |
716 | +# Copyright 2016-2021 Canonical Ltd. This software is licensed under the |
717 | # GNU Affero General Public License version 3 (see the file LICENSE). |
718 | |
719 | """Snap build jobs.""" |
720 | @@ -241,6 +241,29 @@ class SnapStoreUploadJob(SnapBuildJobDerived): |
721 | self.metadata["error_messages"] = messages |
722 | |
723 | @property |
724 | + def upload_id(self): |
725 | + """See `ISnapStoreUploadJob`.""" |
726 | + return self.store_metadata.get("upload_id") |
727 | + |
728 | + @upload_id.setter |
729 | + def upload_id(self, upload_id): |
730 | + """See `ISnapStoreUploadJob`.""" |
731 | + if self.snapbuild.store_upload_metadata is None: |
732 | + self.snapbuild.store_upload_metadata = {} |
733 | + self.snapbuild.store_upload_metadata["upload_id"] = upload_id |
734 | + |
735 | + @property |
736 | + def status_url(self): |
737 | + """See `ISnapStoreUploadJob`.""" |
738 | + return self.store_metadata.get("status_url") |
739 | + |
740 | + @status_url.setter |
741 | + def status_url(self, url): |
742 | + if self.snapbuild.store_upload_metadata is None: |
743 | + self.snapbuild.store_upload_metadata = {} |
744 | + self.snapbuild.store_upload_metadata["status_url"] = url |
745 | + |
746 | + @property |
747 | def store_url(self): |
748 | """See `ISnapStoreUploadJob`.""" |
749 | return self.store_metadata.get("store_url") |
750 | @@ -265,17 +288,6 @@ class SnapStoreUploadJob(SnapBuildJobDerived): |
751 | self.snapbuild.store_upload_metadata["store_revision"] = revision |
752 | self.snapbuild._store_upload_revision = revision |
753 | |
754 | - @property |
755 | - def status_url(self): |
756 | - """See `ISnapStoreUploadJob`.""" |
757 | - return self.store_metadata.get('status_url') |
758 | - |
759 | - @status_url.setter |
760 | - def status_url(self, url): |
761 | - if self.snapbuild.store_upload_metadata is None: |
762 | - self.snapbuild.store_upload_metadata = {} |
763 | - self.snapbuild.store_upload_metadata["status_url"] = url |
764 | - |
765 | # Ideally we'd just override Job._set_status or similar, but |
766 | # lazr.delegates makes that difficult, so we use this to override all |
767 | # the individual Job lifecycle methods instead. |
768 | @@ -333,8 +345,18 @@ class SnapStoreUploadJob(SnapBuildJobDerived): |
769 | client = getUtility(ISnapStoreClient) |
770 | try: |
771 | try: |
772 | + lfa = next((row[1] for row in self.snapbuild.getFiles()), None) |
773 | + if lfa is None: |
774 | + # Nothing to do. |
775 | + self.error_message = None |
776 | + return |
777 | + if "upload_id" not in self.store_metadata: |
778 | + self.upload_id = client.uploadFile(lfa) |
779 | + # We made progress, so reset attempt_count. |
780 | + self.attempt_count = 1 |
781 | if "status_url" not in self.store_metadata: |
782 | - self.status_url = client.upload(self.snapbuild) |
783 | + self.status_url = client.push( |
784 | + self.snapbuild, self.upload_id) |
785 | # We made progress, so reset attempt_count. |
786 | self.attempt_count = 1 |
787 | if self.store_url is None: |
788 | diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py |
789 | index 3d1e65d..91b05c5 100644 |
790 | --- a/lib/lp/snappy/model/snapstoreclient.py |
791 | +++ b/lib/lp/snappy/model/snapstoreclient.py |
792 | @@ -237,7 +237,7 @@ class SnapStoreClient: |
793 | timeline_action.finish() |
794 | |
795 | @classmethod |
796 | - def _uploadFile(cls, lfa, lfc): |
797 | + def uploadFile(cls, lfa): |
798 | """Upload a single file.""" |
799 | assert config.snappy.store_upload_url is not None |
800 | unscanned_upload_url = urlappend( |
801 | @@ -262,24 +262,22 @@ class SnapStoreClient: |
802 | response_data = response.json() |
803 | if not response_data.get("successful", False): |
804 | raise UploadFailedResponse(response.text) |
805 | - return {"upload_id": response_data["upload_id"]} |
806 | + return response_data["upload_id"] |
807 | except requests.HTTPError as e: |
808 | raise cls._makeSnapStoreError(UploadFailedResponse, e) |
809 | finally: |
810 | lfa.close() |
811 | |
812 | @classmethod |
813 | - def _uploadApp(cls, snapbuild, upload_data): |
814 | + def _push(cls, snapbuild, upload_id): |
815 | """Create a new store upload based on the uploaded file.""" |
816 | snap = snapbuild.snap |
817 | - assert config.snappy.store_url is not None |
818 | - assert snap.store_name is not None |
819 | - assert snap.store_secrets is not None |
820 | + assert snap.can_upload_to_store |
821 | assert snapbuild.date_started is not None |
822 | upload_url = urlappend(config.snappy.store_url, "dev/api/snap-push/") |
823 | data = { |
824 | "name": snap.store_name, |
825 | - "updown_id": upload_data["upload_id"], |
826 | + "updown_id": upload_id, |
827 | "series": snap.store_series.name, |
828 | "built_at": snapbuild.date_started.isoformat(), |
829 | } |
830 | @@ -312,15 +310,10 @@ class SnapStoreClient: |
831 | raise cls._makeSnapStoreError(UploadFailedResponse, e) |
832 | |
833 | @classmethod |
834 | - def upload(cls, snapbuild): |
835 | + def push(cls, snapbuild, upload_id): |
836 | """See `ISnapStoreClient`.""" |
837 | - assert snapbuild.snap.can_upload_to_store |
838 | - for _, lfa, lfc in snapbuild.getFiles(): |
839 | - if not lfa.filename.endswith(".snap"): |
840 | - continue |
841 | - upload_data = cls._uploadFile(lfa, lfc) |
842 | - return cls.refreshIfNecessary( |
843 | - snapbuild.snap, cls._uploadApp, snapbuild, upload_data) |
844 | + return cls.refreshIfNecessary( |
845 | + snapbuild.snap, cls._push, snapbuild, upload_id) |
846 | |
847 | @classmethod |
848 | def refreshDischargeMacaroon(cls, snap): |
849 | diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py |
850 | index 5b56264..1c1f316 100644 |
851 | --- a/lib/lp/snappy/tests/test_snapbuildjob.py |
852 | +++ b/lib/lp/snappy/tests/test_snapbuildjob.py |
853 | @@ -1,4 +1,4 @@ |
854 | -# Copyright 2016-2019 Canonical Ltd. This software is licensed under the |
855 | +# Copyright 2016-2021 Canonical Ltd. This software is licensed under the |
856 | # GNU Affero General Public License version 3 (see the file LICENSE). |
857 | |
858 | """Tests for snap build jobs.""" |
859 | @@ -73,7 +73,8 @@ def run_isolated_jobs(jobs): |
860 | class FakeSnapStoreClient: |
861 | |
862 | def __init__(self): |
863 | - self.upload = FakeMethod() |
864 | + self.uploadFile = FakeMethod() |
865 | + self.push = FakeMethod() |
866 | self.checkStatus = FakeMethod() |
867 | self.listChannels = FakeMethod(result=[]) |
868 | |
869 | @@ -120,10 +121,13 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
870 | repr(job)) |
871 | |
872 | def makeSnapBuild(self, **kwargs): |
873 | - # Make a build with a builder and a webhook. |
874 | + # Make a build with a builder, a file, and a webhook. |
875 | snapbuild = self.factory.makeSnapBuild( |
876 | builder=self.factory.makeBuilder(), **kwargs) |
877 | snapbuild.updateStatus(BuildStatus.FULLYBUILT) |
878 | + snap_lfa = self.factory.makeLibraryFileAlias( |
879 | + filename="test-snap.snap", content=b"dummy snap content") |
880 | + self.factory.makeSnapFile(snapbuild=snapbuild, libraryfile=snap_lfa) |
881 | self.factory.makeWebhook( |
882 | target=snapbuild.snap, event_types=["snap:build:0.1"]) |
883 | return snapbuild |
884 | @@ -165,15 +169,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
885 | # and revision. |
886 | logger = self.useFixture(FakeLogger()) |
887 | snapbuild = self.makeSnapBuild() |
888 | + snap_lfa = snapbuild.getFiles()[0][1] |
889 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
890 | job = SnapStoreUploadJob.create(snapbuild) |
891 | client = FakeSnapStoreClient() |
892 | - client.upload.result = self.status_url |
893 | + client.uploadFile.result = 1 |
894 | + client.push.result = self.status_url |
895 | client.checkStatus.result = (self.store_url, 1) |
896 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
897 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
898 | run_isolated_jobs([job]) |
899 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
900 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
901 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
902 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
903 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
904 | self.assertEqual(self.store_url, job.store_url) |
905 | @@ -189,14 +196,16 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
906 | # A failed run sets the store upload status to FAILED. |
907 | logger = self.useFixture(FakeLogger()) |
908 | snapbuild = self.makeSnapBuild() |
909 | + snap_lfa = snapbuild.getFiles()[0][1] |
910 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
911 | job = SnapStoreUploadJob.create(snapbuild) |
912 | client = FakeSnapStoreClient() |
913 | - client.upload.failure = ValueError("An upload failure") |
914 | + client.uploadFile.failure = ValueError("An upload failure") |
915 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
916 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
917 | run_isolated_jobs([job]) |
918 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
919 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
920 | + self.assertEqual([], client.push.calls) |
921 | self.assertEqual([], client.checkStatus.calls) |
922 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
923 | self.assertIsNone(job.store_url) |
924 | @@ -216,15 +225,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
925 | owner=requester, name="requester-team", members=[requester]) |
926 | snapbuild = self.makeSnapBuild( |
927 | requester=requester_team, name="test-snap", owner=requester_team) |
928 | + snap_lfa = snapbuild.getFiles()[0][1] |
929 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
930 | job = SnapStoreUploadJob.create(snapbuild) |
931 | client = FakeSnapStoreClient() |
932 | - client.upload.failure = UnauthorizedUploadResponse( |
933 | + client.uploadFile.result = 1 |
934 | + client.push.failure = UnauthorizedUploadResponse( |
935 | "Authorization failed.") |
936 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
937 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
938 | run_isolated_jobs([job]) |
939 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
940 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
941 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
942 | self.assertEqual([], client.checkStatus.calls) |
943 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
944 | self.assertIsNone(job.store_url) |
945 | @@ -265,15 +277,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
946 | # retried. |
947 | logger = self.useFixture(FakeLogger()) |
948 | snapbuild = self.makeSnapBuild() |
949 | + snap_lfa = snapbuild.getFiles()[0][1] |
950 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
951 | job = SnapStoreUploadJob.create(snapbuild) |
952 | client = FakeSnapStoreClient() |
953 | - client.upload.failure = UploadFailedResponse( |
954 | + client.uploadFile.failure = UploadFailedResponse( |
955 | "Proxy error", can_retry=True) |
956 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
957 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
958 | run_isolated_jobs([job]) |
959 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
960 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
961 | + self.assertEqual([], client.push.calls) |
962 | self.assertEqual([], client.checkStatus.calls) |
963 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
964 | self.assertIsNone(job.store_url) |
965 | @@ -287,13 +301,15 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
966 | # Try again. The upload part of the job is retried, and this time |
967 | # it succeeds. |
968 | job.scheduled_start = None |
969 | - client.upload.calls = [] |
970 | - client.upload.failure = None |
971 | - client.upload.result = self.status_url |
972 | + client.uploadFile.calls = [] |
973 | + client.uploadFile.failure = None |
974 | + client.uploadFile.result = 1 |
975 | + client.push.result = self.status_url |
976 | client.checkStatus.result = (self.store_url, 1) |
977 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
978 | run_isolated_jobs([job]) |
979 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
980 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
981 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
982 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
983 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
984 | self.assertEqual(self.store_url, job.store_url) |
985 | @@ -315,14 +331,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
986 | owner=requester, name="requester-team", members=[requester]) |
987 | snapbuild = self.makeSnapBuild( |
988 | requester=requester_team, name="test-snap", owner=requester_team) |
989 | + snap_lfa = snapbuild.getFiles()[0][1] |
990 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
991 | job = SnapStoreUploadJob.create(snapbuild) |
992 | client = FakeSnapStoreClient() |
993 | - client.upload.failure = BadRefreshResponse("SSO melted.") |
994 | + client.uploadFile.result = 1 |
995 | + client.push.failure = BadRefreshResponse("SSO melted.") |
996 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
997 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
998 | run_isolated_jobs([job]) |
999 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1000 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1001 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
1002 | self.assertEqual([], client.checkStatus.calls) |
1003 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1004 | self.assertIsNone(job.store_url) |
1005 | @@ -368,15 +387,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1006 | owner=requester, name="requester-team", members=[requester]) |
1007 | snapbuild = self.makeSnapBuild( |
1008 | requester=requester_team, name="test-snap", owner=requester_team) |
1009 | + snap_lfa = snapbuild.getFiles()[0][1] |
1010 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1011 | job = SnapStoreUploadJob.create(snapbuild) |
1012 | client = FakeSnapStoreClient() |
1013 | - client.upload.failure = UploadFailedResponse( |
1014 | + client.uploadFile.failure = UploadFailedResponse( |
1015 | "Failed to upload", detail="The proxy exploded.\n") |
1016 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1017 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1018 | run_isolated_jobs([job]) |
1019 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1020 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1021 | + self.assertEqual([], client.push.calls) |
1022 | self.assertEqual([], client.checkStatus.calls) |
1023 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1024 | self.assertIsNone(job.store_url) |
1025 | @@ -420,15 +441,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1026 | # package schedules itself to be retried. |
1027 | logger = self.useFixture(FakeLogger()) |
1028 | snapbuild = self.makeSnapBuild() |
1029 | + snap_lfa = snapbuild.getFiles()[0][1] |
1030 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1031 | job = SnapStoreUploadJob.create(snapbuild) |
1032 | client = FakeSnapStoreClient() |
1033 | - client.upload.result = self.status_url |
1034 | + client.uploadFile.result = 2 |
1035 | + client.push.result = self.status_url |
1036 | client.checkStatus.failure = UploadNotScannedYetResponse() |
1037 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1038 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1039 | run_isolated_jobs([job]) |
1040 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1041 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1042 | + self.assertEqual([((snapbuild, 2), {})], client.push.calls) |
1043 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
1044 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1045 | self.assertIsNone(job.store_url) |
1046 | @@ -439,16 +463,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1047 | self.assertEqual([], pop_notifications()) |
1048 | self.assertEqual(JobStatus.WAITING, job.job.status) |
1049 | self.assertWebhookDeliveries(snapbuild, ["Pending"], logger) |
1050 | - # Try again. The upload part of the job is not retried, and this |
1051 | - # time the scan completes. |
1052 | + # Try again. The upload and push parts of the job are not retried, |
1053 | + # and this time the scan completes. |
1054 | job.scheduled_start = None |
1055 | - client.upload.calls = [] |
1056 | + client.uploadFile.calls = [] |
1057 | + client.push.calls = [] |
1058 | client.checkStatus.calls = [] |
1059 | client.checkStatus.failure = None |
1060 | client.checkStatus.result = (self.store_url, 1) |
1061 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1062 | run_isolated_jobs([job]) |
1063 | - self.assertEqual([], client.upload.calls) |
1064 | + self.assertEqual([], client.uploadFile.calls) |
1065 | + self.assertEqual([], client.push.calls) |
1066 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
1067 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1068 | self.assertEqual(self.store_url, job.store_url) |
1069 | @@ -469,10 +495,12 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1070 | owner=requester, name="requester-team", members=[requester]) |
1071 | snapbuild = self.makeSnapBuild( |
1072 | requester=requester_team, name="test-snap", owner=requester_team) |
1073 | + snap_lfa = snapbuild.getFiles()[0][1] |
1074 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1075 | job = SnapStoreUploadJob.create(snapbuild) |
1076 | client = FakeSnapStoreClient() |
1077 | - client.upload.result = self.status_url |
1078 | + client.uploadFile.result = 2 |
1079 | + client.push.result = self.status_url |
1080 | client.checkStatus.failure = ScanFailedResponse( |
1081 | "Scan failed.\nConfinement not allowed.", |
1082 | messages=[ |
1083 | @@ -481,7 +509,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1084 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1085 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1086 | run_isolated_jobs([job]) |
1087 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1088 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1089 | + self.assertEqual([((snapbuild, 2), {})], client.push.calls) |
1090 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
1091 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1092 | self.assertIsNone(job.store_url) |
1093 | @@ -526,15 +555,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1094 | # URL or revision. |
1095 | logger = self.useFixture(FakeLogger()) |
1096 | snapbuild = self.makeSnapBuild() |
1097 | + snap_lfa = snapbuild.getFiles()[0][1] |
1098 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1099 | job = SnapStoreUploadJob.create(snapbuild) |
1100 | client = FakeSnapStoreClient() |
1101 | - client.upload.result = self.status_url |
1102 | + client.uploadFile.result = 1 |
1103 | + client.push.result = self.status_url |
1104 | client.checkStatus.result = (None, None) |
1105 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1106 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1107 | run_isolated_jobs([job]) |
1108 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1109 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1110 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
1111 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
1112 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1113 | self.assertIsNone(job.store_url) |
1114 | @@ -551,15 +583,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1115 | # channels does so. |
1116 | logger = self.useFixture(FakeLogger()) |
1117 | snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"]) |
1118 | + snap_lfa = snapbuild.getFiles()[0][1] |
1119 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1120 | job = SnapStoreUploadJob.create(snapbuild) |
1121 | client = FakeSnapStoreClient() |
1122 | - client.upload.result = self.status_url |
1123 | + client.uploadFile.result = 1 |
1124 | + client.push.result = self.status_url |
1125 | client.checkStatus.result = (self.store_url, 1) |
1126 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1127 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1128 | run_isolated_jobs([job]) |
1129 | - self.assertEqual([((snapbuild,), {})], client.upload.calls) |
1130 | + self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls) |
1131 | + self.assertEqual([((snapbuild, 1), {})], client.push.calls) |
1132 | self.assertEqual([((self.status_url,), {})], client.checkStatus.calls) |
1133 | self.assertContentEqual([job], snapbuild.store_upload_jobs) |
1134 | self.assertEqual(self.store_url, job.store_url) |
1135 | @@ -579,16 +614,26 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1136 | snapbuild = self.makeSnapBuild() |
1137 | job = SnapStoreUploadJob.create(snapbuild) |
1138 | client = FakeSnapStoreClient() |
1139 | - client.upload.failure = UploadFailedResponse( |
1140 | + client.uploadFile.failure = UploadFailedResponse( |
1141 | "Proxy error", can_retry=True) |
1142 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1143 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1144 | run_isolated_jobs([job]) |
1145 | - self.assertNotIn("status_url", job.metadata) |
1146 | + self.assertNotIn("upload_id", job.store_metadata) |
1147 | self.assertEqual(timedelta(seconds=60), job.retry_delay) |
1148 | job.scheduled_start = None |
1149 | - client.upload.failure = None |
1150 | - client.upload.result = self.status_url |
1151 | + client.uploadFile.failure = None |
1152 | + client.uploadFile.result = 1 |
1153 | + client.push.failure = UploadFailedResponse( |
1154 | + "Proxy error", can_retry=True) |
1155 | + with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1156 | + run_isolated_jobs([job]) |
1157 | + self.assertIn("upload_id", job.snapbuild.store_upload_metadata) |
1158 | + self.assertNotIn("status_url", job.store_metadata) |
1159 | + self.assertEqual(timedelta(seconds=60), job.retry_delay) |
1160 | + job.scheduled_start = None |
1161 | + client.push.failure = None |
1162 | + client.push.result = self.status_url |
1163 | client.checkStatus.failure = UploadNotScannedYetResponse() |
1164 | for expected_delay in (15, 15, 30, 30, 60): |
1165 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1166 | @@ -608,39 +653,88 @@ class TestSnapStoreUploadJob(TestCaseWithFactory): |
1167 | self.assertEqual(JobStatus.COMPLETED, job.job.status) |
1168 | |
1169 | def test_retry_after_upload_does_not_upload(self): |
1170 | - # If the job has uploaded, but failed to release, it should |
1171 | - # not attempt to upload again on the next run. |
1172 | + # If the job has uploaded but failed to push, it should not attempt |
1173 | + # to upload again on the next run. |
1174 | self.useFixture(FakeLogger()) |
1175 | snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"]) |
1176 | self.assertContentEqual([], snapbuild.store_upload_jobs) |
1177 | job = SnapStoreUploadJob.create(snapbuild) |
1178 | client = FakeSnapStoreClient() |
1179 | - client.upload.result = self.status_url |
1180 | - client.checkStatus.result = (self.store_url, 1) |
1181 | + client.uploadFile.result = 1 |
1182 | + client.push.failure = UploadFailedResponse( |
1183 | + "Proxy error", can_retry=True) |
1184 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1185 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1186 | run_isolated_jobs([job]) |
1187 | |
1188 | - previous_upload = client.upload.calls |
1189 | - previous_checkStatus = client.checkStatus.calls |
1190 | + # We performed the upload as expected, but the push failed. |
1191 | + self.assertIsNone(job.store_url) |
1192 | + self.assertIsNone(job.store_revision) |
1193 | + self.assertIsNone( |
1194 | + removeSecurityProxy(snapbuild)._store_upload_revision) |
1195 | + self.assertEqual(timedelta(seconds=60), job.retry_delay) |
1196 | + self.assertEqual(1, len(client.uploadFile.calls)) |
1197 | + self.assertIsNone(job.error_message) |
1198 | + |
1199 | + # Run the job again. |
1200 | + client.uploadFile.calls = [] |
1201 | + client.push.calls = [] |
1202 | + client.push.failure = None |
1203 | + client.push.result = self.status_url |
1204 | + client.checkStatus.result = (self.store_url, 1) |
1205 | + with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1206 | + run_isolated_jobs([job]) |
1207 | |
1208 | - # Check we uploaded as expected |
1209 | + # The push has now succeeded. Make sure that we didn't try to |
1210 | + # upload the file again first. |
1211 | self.assertEqual(self.store_url, job.store_url) |
1212 | self.assertEqual(1, job.store_revision) |
1213 | - self.assertEqual(1, |
1214 | - removeSecurityProxy(snapbuild)._store_upload_revision) |
1215 | - self.assertEqual(timedelta(seconds=60), job.retry_delay) |
1216 | - self.assertEqual(1, len(client.upload.calls)) |
1217 | + self.assertEqual([], client.uploadFile.calls) |
1218 | + self.assertEqual(1, len(client.push.calls)) |
1219 | self.assertIsNone(job.error_message) |
1220 | |
1221 | - # Run the job again |
1222 | + def test_retry_after_push_does_not_upload_or_push(self): |
1223 | + # If the job has uploaded and pushed but has not yet been scanned, |
1224 | + # it should not attempt to upload or push again on the next run. |
1225 | + self.useFixture(FakeLogger()) |
1226 | + snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"]) |
1227 | + self.assertContentEqual([], snapbuild.store_upload_jobs) |
1228 | + job = SnapStoreUploadJob.create(snapbuild) |
1229 | + client = FakeSnapStoreClient() |
1230 | + client.uploadFile.result = 1 |
1231 | + client.push.result = self.status_url |
1232 | + client.checkStatus.failure = UploadNotScannedYetResponse() |
1233 | + self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) |
1234 | + with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1235 | + run_isolated_jobs([job]) |
1236 | + |
1237 | + # We performed the upload and push as expected, but the store is |
1238 | + # still scanning it. |
1239 | + self.assertIsNone(job.store_url) |
1240 | + self.assertIsNone(job.store_revision) |
1241 | + self.assertIsNone( |
1242 | + removeSecurityProxy(snapbuild)._store_upload_revision) |
1243 | + self.assertEqual(timedelta(seconds=15), job.retry_delay) |
1244 | + self.assertEqual(1, len(client.uploadFile.calls)) |
1245 | + self.assertEqual(1, len(client.push.calls)) |
1246 | + self.assertIsNone(job.error_message) |
1247 | + |
1248 | + # Run the job again. |
1249 | + client.uploadFile.calls = [] |
1250 | + client.push.calls = [] |
1251 | + client.checkStatus.calls = [] |
1252 | + client.checkStatus.failure = None |
1253 | + client.checkStatus.result = (self.store_url, 1) |
1254 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1255 | run_isolated_jobs([job]) |
1256 | |
1257 | - # Release is not called due to release intent in upload |
1258 | - # but ensure that we have not called upload twice |
1259 | - self.assertEqual(previous_upload, client.upload.calls) |
1260 | - self.assertEqual(previous_checkStatus, client.checkStatus.calls) |
1261 | + # The store has now scanned the upload. Make sure that we didn't |
1262 | + # try to upload or push it again first. |
1263 | + self.assertEqual(self.store_url, job.store_url) |
1264 | + self.assertEqual(1, job.store_revision) |
1265 | + self.assertEqual([], client.uploadFile.calls) |
1266 | + self.assertEqual([], client.push.calls) |
1267 | + self.assertEqual(1, len(client.checkStatus.calls)) |
1268 | self.assertIsNone(job.error_message) |
1269 | |
1270 | def test_with_snapbuild_metadata_as_none(self): |
1271 | diff --git a/lib/lp/snappy/tests/test_snapstoreclient.py b/lib/lp/snappy/tests/test_snapstoreclient.py |
1272 | index c738171..3cc26e2 100644 |
1273 | --- a/lib/lp/snappy/tests/test_snapstoreclient.py |
1274 | +++ b/lib/lp/snappy/tests/test_snapstoreclient.py |
1275 | @@ -361,15 +361,13 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1276 | return snapbuild |
1277 | |
1278 | @responses.activate |
1279 | - def test_upload(self): |
1280 | - snapbuild = self.makeUploadableSnapBuild() |
1281 | + def test_uploadFile(self): |
1282 | + snap_lfa = self.factory.makeLibraryFileAlias( |
1283 | + filename="test-snap.snap", content=b"dummy snap content") |
1284 | transaction.commit() |
1285 | self._addUnscannedUploadResponse() |
1286 | - self._addSnapPushResponse() |
1287 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1288 | - self.assertEqual( |
1289 | - "http://sca.example/dev/api/snaps/1/builds/1/status", |
1290 | - self.client.upload(snapbuild)) |
1291 | + self.assertEqual(1, self.client.uploadFile(snap_lfa)) |
1292 | requests = [call.request for call in responses.calls] |
1293 | self.assertThat(requests, MatchesListwise([ |
1294 | RequestMatches( |
1295 | @@ -381,6 +379,34 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1296 | value=b"dummy snap content", |
1297 | type="application/octet-stream", |
1298 | )}), |
1299 | + ])) |
1300 | + |
1301 | + @responses.activate |
1302 | + def test_uploadFile_error(self): |
1303 | + snap_lfa = self.factory.makeLibraryFileAlias( |
1304 | + filename="test-snap.snap", content=b"dummy snap content") |
1305 | + transaction.commit() |
1306 | + responses.add( |
1307 | + "POST", "http://updown.example/unscanned-upload/", status=502, |
1308 | + body="The proxy exploded.\n") |
1309 | + with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1310 | + err = self.assertRaises( |
1311 | + UploadFailedResponse, self.client.uploadFile, snap_lfa) |
1312 | + self.assertEqual("502 Server Error: Bad Gateway", str(err)) |
1313 | + self.assertEqual("The proxy exploded.\n", err.detail) |
1314 | + self.assertTrue(err.can_retry) |
1315 | + |
1316 | + @responses.activate |
1317 | + def test_push(self): |
1318 | + snapbuild = self.makeUploadableSnapBuild() |
1319 | + transaction.commit() |
1320 | + self._addSnapPushResponse() |
1321 | + with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1322 | + self.assertEqual( |
1323 | + "http://sca.example/dev/api/snaps/1/builds/1/status", |
1324 | + self.client.push(snapbuild, 1)) |
1325 | + requests = [call.request for call in responses.calls] |
1326 | + self.assertThat(requests, MatchesListwise([ |
1327 | RequestMatches( |
1328 | url=Equals("http://sca.example/dev/api/snap-push/"), |
1329 | method=Equals("POST"), |
1330 | @@ -394,28 +420,18 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1331 | ])) |
1332 | |
1333 | @responses.activate |
1334 | - def test_upload_with_release_intent(self): |
1335 | + def test_push_with_release_intent(self): |
1336 | snapbuild = self.makeUploadableSnapBuild() |
1337 | snapbuild.snap.store_channels = ['beta', 'edge'] |
1338 | transaction.commit() |
1339 | - self._addUnscannedUploadResponse() |
1340 | self._addSnapPushResponse() |
1341 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1342 | self.assertEqual( |
1343 | "http://sca.example/dev/api/snaps/1/builds/1/status", |
1344 | - self.client.upload(snapbuild)) |
1345 | + self.client.push(snapbuild, 1)) |
1346 | requests = [call.request for call in responses.calls] |
1347 | self.assertThat(requests, MatchesListwise([ |
1348 | RequestMatches( |
1349 | - url=Equals("http://updown.example/unscanned-upload/"), |
1350 | - method=Equals("POST"), |
1351 | - form_data={ |
1352 | - "binary": MatchesStructure.byEquality( |
1353 | - name="binary", filename="test-snap.snap", |
1354 | - value=b"dummy snap content", |
1355 | - type="application/octet-stream", |
1356 | - )}), |
1357 | - RequestMatches( |
1358 | url=Equals("http://sca.example/dev/api/snap-push/"), |
1359 | method=Equals("POST"), |
1360 | headers=ContainsDict( |
1361 | @@ -429,30 +445,20 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1362 | ])) |
1363 | |
1364 | @responses.activate |
1365 | - def test_upload_no_discharge(self): |
1366 | + def test_push_no_discharge(self): |
1367 | root_key = hashlib.sha256(self.factory.getUniqueBytes()).hexdigest() |
1368 | root_macaroon = Macaroon(key=root_key) |
1369 | snapbuild = self.makeUploadableSnapBuild( |
1370 | store_secrets={"root": root_macaroon.serialize()}) |
1371 | transaction.commit() |
1372 | - self._addUnscannedUploadResponse() |
1373 | self._addSnapPushResponse() |
1374 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1375 | self.assertEqual( |
1376 | "http://sca.example/dev/api/snaps/1/builds/1/status", |
1377 | - self.client.upload(snapbuild)) |
1378 | + self.client.push(snapbuild, 1)) |
1379 | requests = [call.request for call in responses.calls] |
1380 | self.assertThat(requests, MatchesListwise([ |
1381 | RequestMatches( |
1382 | - url=Equals("http://updown.example/unscanned-upload/"), |
1383 | - method=Equals("POST"), |
1384 | - form_data={ |
1385 | - "binary": MatchesStructure.byEquality( |
1386 | - name="binary", filename="test-snap.snap", |
1387 | - value=b"dummy snap content", |
1388 | - type="application/octet-stream", |
1389 | - )}), |
1390 | - RequestMatches( |
1391 | url=Equals("http://sca.example/dev/api/snap-push/"), |
1392 | method=Equals("POST"), |
1393 | headers=ContainsDict( |
1394 | @@ -465,7 +471,7 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1395 | ])) |
1396 | |
1397 | @responses.activate |
1398 | - def test_upload_encrypted_discharge(self): |
1399 | + def test_push_encrypted_discharge(self): |
1400 | private_key = PrivateKey.generate() |
1401 | self.pushConfig( |
1402 | "snappy", |
1403 | @@ -475,24 +481,14 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1404 | bytes(private_key)).decode("UTF-8")) |
1405 | snapbuild = self.makeUploadableSnapBuild(encrypted=True) |
1406 | transaction.commit() |
1407 | - self._addUnscannedUploadResponse() |
1408 | self._addSnapPushResponse() |
1409 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1410 | self.assertEqual( |
1411 | "http://sca.example/dev/api/snaps/1/builds/1/status", |
1412 | - self.client.upload(snapbuild)) |
1413 | + self.client.push(snapbuild, 1)) |
1414 | requests = [call.request for call in responses.calls] |
1415 | self.assertThat(requests, MatchesListwise([ |
1416 | RequestMatches( |
1417 | - url=Equals("http://updown.example/unscanned-upload/"), |
1418 | - method=Equals("POST"), |
1419 | - form_data={ |
1420 | - "binary": MatchesStructure.byEquality( |
1421 | - name="binary", filename="test-snap.snap", |
1422 | - value=b"dummy snap content", |
1423 | - type="application/octet-stream", |
1424 | - )}), |
1425 | - RequestMatches( |
1426 | url=Equals("http://sca.example/dev/api/snap-push/"), |
1427 | method=Equals("POST"), |
1428 | headers=ContainsDict( |
1429 | @@ -505,11 +501,10 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1430 | ])) |
1431 | |
1432 | @responses.activate |
1433 | - def test_upload_unauthorized(self): |
1434 | + def test_push_unauthorized(self): |
1435 | store_secrets = self._make_store_secrets() |
1436 | snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets) |
1437 | transaction.commit() |
1438 | - self._addUnscannedUploadResponse() |
1439 | snap_push_error = { |
1440 | "code": "macaroon-permission-required", |
1441 | "message": "Permission is required: package_push", |
1442 | @@ -522,14 +517,13 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1443 | self.assertRaisesWithContent( |
1444 | UnauthorizedUploadResponse, |
1445 | "Permission is required: package_push", |
1446 | - self.client.upload, snapbuild) |
1447 | + self.client.push, snapbuild, 1) |
1448 | |
1449 | @responses.activate |
1450 | - def test_upload_needs_discharge_macaroon_refresh(self): |
1451 | + def test_push_needs_discharge_macaroon_refresh(self): |
1452 | store_secrets = self._make_store_secrets() |
1453 | snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets) |
1454 | transaction.commit() |
1455 | - self._addUnscannedUploadResponse() |
1456 | responses.add( |
1457 | "POST", "http://sca.example/dev/api/snap-push/", status=401, |
1458 | headers={"WWW-Authenticate": "Macaroon needs_refresh=1"}) |
1459 | @@ -538,10 +532,9 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1460 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1461 | self.assertEqual( |
1462 | "http://sca.example/dev/api/snaps/1/builds/1/status", |
1463 | - self.client.upload(snapbuild)) |
1464 | + self.client.push(snapbuild, 1)) |
1465 | requests = [call.request for call in responses.calls] |
1466 | self.assertThat(requests, MatchesListwise([ |
1467 | - MatchesStructure.byEquality(path_url="/unscanned-upload/"), |
1468 | MatchesStructure.byEquality(path_url="/dev/api/snap-push/"), |
1469 | MatchesStructure.byEquality(path_url="/api/v2/tokens/refresh"), |
1470 | MatchesStructure.byEquality(path_url="/dev/api/snap-push/"), |
1471 | @@ -551,7 +544,7 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1472 | snapbuild.snap.store_secrets["discharge"]) |
1473 | |
1474 | @responses.activate |
1475 | - def test_upload_needs_encrypted_discharge_macaroon_refresh(self): |
1476 | + def test_push_needs_encrypted_discharge_macaroon_refresh(self): |
1477 | private_key = PrivateKey.generate() |
1478 | self.pushConfig( |
1479 | "snappy", |
1480 | @@ -562,7 +555,6 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1481 | store_secrets = self._make_store_secrets(encrypted=True) |
1482 | snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets) |
1483 | transaction.commit() |
1484 | - self._addUnscannedUploadResponse() |
1485 | responses.add( |
1486 | "POST", "http://sca.example/dev/api/snap-push/", status=401, |
1487 | headers={"WWW-Authenticate": "Macaroon needs_refresh=1"}) |
1488 | @@ -571,10 +563,9 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1489 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1490 | self.assertEqual( |
1491 | "http://sca.example/dev/api/snaps/1/builds/1/status", |
1492 | - self.client.upload(snapbuild)) |
1493 | + self.client.push(snapbuild, 1)) |
1494 | requests = [call.request for call in responses.calls] |
1495 | self.assertThat(requests, MatchesListwise([ |
1496 | - MatchesStructure.byEquality(path_url="/unscanned-upload/"), |
1497 | MatchesStructure.byEquality(path_url="/dev/api/snap-push/"), |
1498 | MatchesStructure.byEquality(path_url="/api/v2/tokens/refresh"), |
1499 | MatchesStructure.byEquality(path_url="/dev/api/snap-push/"), |
1500 | @@ -584,37 +575,21 @@ class TestSnapStoreClient(TestCaseWithFactory): |
1501 | snapbuild.snap.store_secrets["discharge_encrypted"]) |
1502 | |
1503 | @responses.activate |
1504 | - def test_upload_unsigned_agreement(self): |
1505 | + def test_push_unsigned_agreement(self): |
1506 | store_secrets = self._make_store_secrets() |
1507 | snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets) |
1508 | transaction.commit() |
1509 | - self._addUnscannedUploadResponse() |
1510 | snap_push_error = {"message": "Developer has not signed agreement."} |
1511 | responses.add( |
1512 | "POST", "http://sca.example/dev/api/snap-push/", status=403, |
1513 | json={"error_list": [snap_push_error]}) |
1514 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1515 | err = self.assertRaises( |
1516 | - UploadFailedResponse, self.client.upload, snapbuild) |
1517 | + UploadFailedResponse, self.client.push, snapbuild, 1) |
1518 | self.assertEqual("Developer has not signed agreement.", str(err)) |
1519 | self.assertFalse(err.can_retry) |
1520 | |
1521 | @responses.activate |
1522 | - def test_upload_file_error(self): |
1523 | - store_secrets = self._make_store_secrets() |
1524 | - snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets) |
1525 | - transaction.commit() |
1526 | - responses.add( |
1527 | - "POST", "http://updown.example/unscanned-upload/", status=502, |
1528 | - body="The proxy exploded.\n") |
1529 | - with dbuser(config.ISnapStoreUploadJobSource.dbuser): |
1530 | - err = self.assertRaises( |
1531 | - UploadFailedResponse, self.client.upload, snapbuild) |
1532 | - self.assertEqual("502 Server Error: Bad Gateway", str(err)) |
1533 | - self.assertEqual("The proxy exploded.\n", err.detail) |
1534 | - self.assertTrue(err.can_retry) |
1535 | - |
1536 | - @responses.activate |
1537 | def test_refresh_discharge_macaroon(self): |
1538 | store_secrets = self._make_store_secrets() |
1539 | snap = self.factory.makeSnap( |
Looks good. Nice test coverage for all the new cases and combination that splitting both operations have added.