Builds: restart build commands before a new build #7999
Conversation
This avoids ending up with duplicate commands. This can be tested by manually triggering a retry in any part of the task. The user may still see duplicate commands, as our js always adds new commands to the end, but after a refresh the correct number of commands are shown.
stsewd
force-pushed
the
fix-dup-commands
branch
from
65c5045 to
1552523
Compare
| @@ -382,6 +382,7 @@ def setUp(self): | |||
| } | |||
| self.response_data = { | |||
| 'build-concurrent': {'status_code': 403}, | |||
| 'build-reset': {'status_code': 403}, | |||
There was a problem hiding this comment.
Not sure if this is useful anymore. Might be owrth removing this automation if we feel we have good test coverage over this stuff.
There was a problem hiding this comment.
Not sure if they are that useful, but it doesn't hurt having more tests p:
| permission_classes=[permissions.IsAdminUser], | ||
| methods=['post'], | ||
| ) | ||
| def reset(self, request, **kwargs): |
There was a problem hiding this comment.
I wonder if this should be a method on the model instead of the API? We can probably keep it here for now, but I could see other cases where it might be useful to have this functionality.
There was a problem hiding this comment.
I'll move it to the model 👍
readthedocs/projects/tasks.py
Outdated
| @@ -670,6 +670,8 @@ def run_setup(self, record=True): | |||
|
|
|||
| Return True if successful. | |||
| """ | |||
| api_v2.build(self.build['id']).reset.post() | |||
There was a problem hiding this comment.
Seems we're calling this on every build? It would be best to run it only in the exception handler, but I'm not 100% sure that code will run properly, so this probably makes sense 👍
There was a problem hiding this comment.
Thinking more, it's probably fine to have it here. There are likely other cases where we want to clear out old build commands (eg. retrying a build if we implement that), so this is probably the most explicit way of handling it.
There was a problem hiding this comment.
Also it would be weird for all commands to disappear till the build is re-triggered. But maybe I can check if the build has any commands build['commands'] before calling the API, let me know if that's better
There was a problem hiding this comment.
If we have that data, it would be a good protection. I just worry about us reseting builds that already have important data in them in the future.
There was a problem hiding this comment.
But I'm not sure if a build with no commands can have other meta-data that could confuse the build
This avoids ending up with duplicate commands.
This can be tested by manually triggering a retry in any part of the
task.
The user may still see duplicate commands,
as our js always adds new commands to the end,
but after a refresh the correct number of commands are shown.