Skip to content
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

Update instance and model when record_as_success #3831

Merged
merged 2 commits into from Mar 22, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Mar 22, 2018

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"

Fixes #3644

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"
@humitos humitos requested a review from agjohnson Mar 22, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Mar 22, 2018

I found another edge case:

When we use record=False, for example to run git status, that command is not recorded in the database, but it's added to the list of command that the BuildEnvironment has executed:

https://github.com/rtfd/readthedocs.org/blob/01bcfe4f1ddcafd62b815ca9bb8a5ca06116ea41/readthedocs/doc_builder/environments.py#L345-L347

So, we have the same problem than I explained in the description: this command is add to the commands list of the instance with exit_code=128 (the repository didn't exist), we don't record it in the db, but then to check update_build we use the commands from the instance:

https://github.com/rtfd/readthedocs.org/blob/01bcfe4f1ddcafd62b815ca9bb8a5ca06116ea41/readthedocs/doc_builder/environments.py#L560-L567

and self.successful returns False in this case because the first command has failed (git status).

Loading

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)
@humitos
Copy link
Member Author

@humitos humitos commented Mar 22, 2018

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.

Loading

@humitos
Copy link
Member Author

@humitos humitos commented Mar 22, 2018

@stsewd could you help me by testing this PR in your local instance and trying to reproduce the issue? Thanks!

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 22, 2018

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

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

Great catch! I'm glad this can be resolved.

LGTM!

Loading

@agjohnson agjohnson merged commit 2a79250 into master Mar 22, 2018
1 check passed
Loading
@agjohnson agjohnson deleted the humitos/build/invalid-finish-state branch Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants