Update Build state at the end of UpdateDocsTask.run #3293
Conversation
Use the `update_build` method that handles most of the cases but fill the `failure` with a unhandled exception in case it was risen.
readthedocs/projects/tasks.py
Outdated
| )) | ||
| finally: | ||
| if unhandled_failure: | ||
| self.build_env.build['failure'] = unhandled_failure |
There was a problem hiding this comment.
Is this supposed to be build['error']?
Also, how is build['error'] = failure being set? Seems this would lose normal build errors, no?
It might be good to put some new tests in to test for these failure cases.
There was a problem hiding this comment.
Is this supposed to be build['error']?
Yes. I got confused with the name.
readthedocs/projects/tasks.py
Outdated
| self.build_env.build['failure'] = unhandled_failure | ||
| self.build_env.update_build(BUILD_STATE_FINISHED) | ||
|
|
||
| return self.get_build(build_pk) |
There was a problem hiding this comment.
It might be best to leave the return logic here, I'm not sure anything is depending on it though
There was a problem hiding this comment.
I think it's the same since the code that was here, was returning the response of the patch request to the build endpoint.
There was a problem hiding this comment.
Oh and i missed that we're not doing the api request here anyways, and can't get the response directly.
Instead of hitting the API again, maybe return the build_env.build object?
There was a problem hiding this comment.
It should be the same, yeah.
readthedocs/projects/tasks.py
Outdated
| @@ -116,6 +116,8 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, | |||
|
|
|||
| This is fully wrapped in exception handling to account for a number of failure cases. | |||
| """ | |||
| unhandled_failure = False | |||
There was a problem hiding this comment.
Its good to store blank string here.
There was a problem hiding this comment.
Why not False?
My idea is "if it's False, nothing happens. If it's the user message, and unhandled exception happens"
There was a problem hiding this comment.
The argument for string is probably that we're treating it as a string below, which will reduce confusion to our future selves :)
There was a problem hiding this comment.
I personally don't like to do if '': but I will change to that if you think it's better.
|
After merging #3292, I'm not sure that this PR makes sense anymore. Since it fixes the same issue but in a different way (a bit cleaner for me -it always update the build status in just one place not matter what has happened). |
|
Yeah, I agree now. I thought that #3292 wasn't overlapping with this. After reading the same code though, I'd like to get this logic in place, it makes much more sense. |
* 'master' of github.com:rtfd/readthedocs.org: Fix shitpost Fix copypasta issue with supervisord config Update Build state at the end of UpdateDocsTask.run (readthedocs#3293)
Use the
update_buildmethod that handles most of the cases but fillthe
failurewith a unhandled exception in case it was risen.Related to #3292