-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Update instance and model when record_as_success
#3831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When we record the command as SUCCESS (exit_code=0) we also need to update the current instance of BuildCommand because `self.exit_code` is used to calculate BuildEnvironment.successful. The later is part of the decision of `update_on_success`, and as the command was marked as failed, we were updating the status. This looked as a random failure, but it was happening always that any of the `record_as_success` command failed, but it wasn't easy to see it because it the jQuery request has to be done exactly after the Build was marked as FAILED and before the next command was executed, since the state was changed immediately to "Building"
I found another edge case: When we use So, we have the same problem than I explained in the description: this command is add to the and |
We want to have consistency between what we have in memory during the build and what we store in the database since we are using one or the other indistinctly to take decisions or show states in the build page. The build was shown as FAILED because `git status` was returning 128 (the repository didn't exist). Although, this command wasn't being recorded in the database, was being used to calculate if the build was successful or not via ``self.successful`` which uses the list of commands (``self.commands`` and their exit codes)
I just fixed the last edge case that I found. This PR will need some review since it seems simple and coeherent but its touching an important chunk of code. Also, the solution proposed here is not the best regarding desing or code style. Although, implementing another solution will take too much refarctoring since we will need to adjust/discuss other concepts probably. |
@stsewd could you help me by testing this PR in your local instance and trying to reproduce the issue? Thanks! |
I was able to reproduce the issue on the current master, but on this branch on several build everything looks good! p.s: kind of I was fixing this on other pr without knowing https://github.com/rtfd/readthedocs.org/pull/3687/files#diff-ca52b098301dd315a834b3556ab9a7d5R363 haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I'm glad this can be resolved.
LGTM!
When we record the command as SUCCESS (exit_code=0) we also need to
update the current instance of BuildCommand because
self.exit_code
is used to calculate BuildEnvironment.successful. The later is part of
the decision of
update_on_success
, and as the command was marked asfailed, we were updating the status.
This looked as a random failure, but it was happening always that any
of the
record_as_success
command failed, but it wasn't easy to seeit because it the jQuery request has to be done exactly after the
Build was marked as FAILED and before the next command was executed,
since the state was changed immediately to "Building"
Fixes #3644