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

Projects
None yet
3 participants
@humitos
Member

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

Update instance and model when `record_as_success`
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

This comment has been minimized.

Member

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:

build_cmd = cls(cmd, **kwargs)
self.commands.append(build_cmd)
build_cmd.run()

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:

update_build = (
# Build isn't done yet, we unconditionally update in this state
not self.done
# Build is done, but isn't successful, always update
or (self.done and not self.successful)
# Otherwise, are we explicitly to not update?
or self.update_on_success
)

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

Add BuildCommand to BaseEnvironment only if record=True
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

This comment has been minimized.

Member

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.

@humitos

This comment has been minimized.

Member

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!

@stsewd

This comment has been minimized.

Member

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

@agjohnson

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

LGTM!

@agjohnson agjohnson merged commit 2a79250 into master Mar 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@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