Merge ~apw/launchpad:archive-removeCopyNotification-fixes into launchpad:master
Proposed by
Andy Whitcroft
Status: | Needs review |
---|---|
Proposed branch: | ~apw/launchpad:archive-removeCopyNotification-fixes |
Merge into: | launchpad:master |
Diff against target: |
34 lines (+3/-2) 2 files modified
lib/lp/soyuz/model/archive.py (+2/-1) lib/lp/soyuz/model/packagecopyjob.py (+1/-1) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Needs Fixing | ||
Review via email: mp+431805@code.launchpad.net |
Commit message
removeCopyNotif
Handle invalid and already acknowledged errors more cleanly reporting NotFound and Gone errors respectively.
To post a comment you must log in.
Unmerged commits
- cbc27c7... by Andy Whitcroft
-
docs:0 (build) lint:0 (build) mypy:0 (build) 1 → 3 of 3 results First • Previous • Next • Last - 06227ef... by Andy Whitcroft
Strictly, I think 410 Gone is really meant to be for cases where the resource identified by the request URL is no longer available, which isn't quite the case here since the archive still exists. Similarly, 404 Not Found would indicate that the archive doesn't exist.
I think we should probably just use 400 Bad Request for this sort of thing. The clearest way to do that would be to invent a new exception type in `lp.soyuz. interfaces. archive` decorated with `@error_ status( http.client. BAD_REQUEST) `; you could use the same exception with a different message for the two cases here if you wanted, sticking a `try:/except NotFoundError:` around the `PlainPackageCo pyJob.get` call to re-raise it as an exception with the right error status decoration.
You'll also need to update tests for these corner cases. It looks as though they mostly already exist in `lp.soyuz. tests.test_ archive. TestRemovingCop yNotifications` , although you might need to add a test for the case of a job ID that doesn't exist at all.
Finally, I wonder if `test_removeCop yNotification_ raises_ for_wrong_ archive` really tests what it claims to be testing. I haven't stepped through it so I might be missing something, but I don't actually see any code that ensures that the job ID is for the context archive; I suspect that the test only passes because the job ID it supplies isn't for a failed job, so it ends up being silently equivalent to `test_removeCop yNotification_ raises_ for_not_ failed` . We'll need to fix this. I'd suggest that all these tests should use `self.assertRai sesWithContent` to assert on the exception string as well as its type; being painfully precise about the exceptions we expect is a good way to avoid this sort of accidental-pass situation.