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

Use gitpython to find a commit reference #3843

Merged
merged 3 commits into from Mar 29, 2018

Conversation

Projects
None yet
5 participants
@agjohnson
Contributor

agjohnson commented Mar 24, 2018

Fixes #3842

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Mar 24, 2018

Test failure is linting failure again.

@agjohnson agjohnson requested a review from rtfd/core Mar 24, 2018

@StanczakDominik

If I can put my 0.02$ in, this seems like a reasonable change that looks promising for fixing the current issues!

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 25, 2018

@agjohnson isn't better to just revert #3831 for now for the hotfix? I think probably others parts of the code depends on the original exit_code, and we don't want more surprises.

@humitos

I think these changes are great, cleaner and nice to read.

Although, I want to mention some things that we need to consider if we want to migrate these simple git commands to a lib based code:

  • how we will execute this inside Docker once we have the VCS code migrated to Docker?
  • how we will present these commands to the user in the build log?
  • if these commands are not needed to be presented to the user, we can just use record=False instead record_as_success=True and avoid these kind of problems
  • since we are taking decisions on the exit_code we will need to check all the other VCS backends to see if this is not affecting them also

I think this PR really solves this particular bug and allows RTD to continue building the docs without problems but we need to consider those points and talk about a migration plan to gitpython/mercurial/bzr

@davidfischer

I'm all for swapping to using a library.

r = Repo(self.working_dir)
if r.commit(ref):
return True
except BadName:

This comment has been minimized.

@davidfischer

davidfischer Mar 26, 2018

Contributor

Perhaps this should be except (BadName, ValueError):. I believe BadName is raised if the string is invalid like a string that can't be a commit hash or isn't a branch. ValueError is raised if a commit is not found for some reason.

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 26, 2018

Here are some others parts of code that depends on the original exit_code

readthedocs/vcs_support/backends/git.py|128 col 1| record_as_success=True,
readthedocs/doc_builder/environments.py|311 col 1| record_as_success=False, **kwargs)
readthedocs/vcs_support/backends/git.py|172 col 1| record_as_success=True,
readthedocs/doc_builder/environments.py|98 col 1| self.record_as_success = record_as_success
readthedocs/vcs_support/backends/hg.py|41 col 1| retcode, stdout = self.run('hg', 'branches', record_as_success=True)[:2]
readthedocs/vcs_support/backends/svn.py|68 col 1| % self.base_url, record_as_success=True)[:2]
readthedocs/vcs_support/backends/git.py|276 col 1| code, _, _ = self.run('git', 'show-ref', ref, record_as_success=True)
readthedocs/vcs_support/backends/bzr.py|48 col 1| retcode, stdout = self.run('bzr', 'tags', record_as_success=True)[:2]
readthedocs/doc_builder/environments.py|75 col 1| bin_path=None, description=None, record_as_success=False)
readthedocs/vcs_support/backends/hg.py|54 col 1| retcode, stdout = self.run('hg', 'tags', record_as_success=True)[:2]
readthedocs/vcs_support/backends/svn.py|37 col 1| retcode = self.run('svn', 'info', record_as_success=True)[0]
@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Mar 27, 2018

I don't think the problem is necessarily that we're using exit_code. I don't think we can remove dependency on that for some of these cases. I'd rather see if we can move these calls to using a library though, as relying on our own parsing and interpretation of exit code for everything is where these bugs come from.

To @humitos questions:

how we will execute this inside Docker once we have the VCS code migrated to Docker?

Not sure yet. Monkey patching command execution to use our docker build command is a possibility. Not running in docker is another. Third would be application code that gets executed from the docker container

how we will present these commands to the user in the build log?

Many of these commands aren't important, there are better ways we can show some of this data. If we can monkey patch, commands will execute as normal

agjohnson added some commits Mar 24, 2018

@agjohnson agjohnson merged commit 7aa6f4d into master Mar 29, 2018

1 check passed

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

@agjohnson agjohnson deleted the agj/hotfix-get-ref branch Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment