Closed mgoerens closed 10 months ago
@mgoerens is the problem here that we're not catching errors returned from this function, or that we're not returning in the case where we do return an error?
It's a bit unclear, it would seem that we're expecting release_id to contain some value in a template, but it doesn't. I'm assuming that it doesn't because release
(in the changed code, which is used to derive release_id
) is not assigned, but I am unsure if the changes here fixes that.
Here is what I understood and how I fixed it:
In the original code, we have a big try
block, with two calls to function that can return an Exception
:
get_release_by_tag
checks that a release exists and return it. If it doesn't exist it raises an Exception
. The release
variable is assigned with what's returned from this function call. It can be unassigned if the function raised an Exception
.check_release_assets
checks if the required assets have been uploaded as part of the release, and raises an Exception
if it doesn't.The finally
block deletes the release. It has been written to handle the second case (if a release exists but doesn't contain the required assets) and the happy path (if a release exists and does contain the required assets). If the get_release_by_tag
function raises an Exception
(i.e. the release doesn't exist), then it fails because the release
variable is unassigned. Anyway, it doesn't make sense to try to delete a release that doesn't exist.
The changes I bring split the try
block in two parts:
get_release_by_tag
, without a finally
block. If the release doesn't exist, just raise an AssertionError.check_release_assets
, with a finally
block that ensures the release (which for sure exists) is being deleted no matter the result of that check.
Closes #291