Silence the stderr git warning when running from a non-git directory

mzsanford commented 7 years ago

When running from a non-git directory the process will print fatal: Not a git repository (or any of the parent directories): .git to stderr once every minute or so. In our case we use a carefully packaged Docker container which excludes the git directory to keep the sizes smaller. This pull request silences that warning and maintains the same functionality (returning None)

A test was also added to reproduce the issue while troubleshooting, though it was unable to capture the existing subprocess stderr so could not assert there will not be a regression. The new test will, however, display the fatal: Not a git repository (or any of the parent directories): .git warning when running test tests if there is a regression in the future.

phumpal commented 7 years ago

@mzsanford thanks for this. I've confirmed I have the same issue both in and out of Docker image builds (from a non-git path).

I don't necessarily think we can solve this any differently. Please rebase when you get a chance.

@zachgoldstein PTAL.

mzsanford commented 7 years ago

@phumpal what would you like me to rebase against? I don't see any changes to master since I branched. I did notice my change introduced a Circle CI failure. The best I can tell that is because I changed a failure case from returning 'None' (a string) to None.

I don't know Circle CI very well but I can attempt to fix that if you would like. It will probably end up generating some build notifications as I brute force debug the Circle CI environment so I wanted to give you a heads up. Let me know if there is a specific ref you want me to rebase against and if you would like me to look into the Circle CI failure.

zachgoldstein commented 7 years ago

@mzsanford thanks for the PR! This has been bugging me for a while, so definitely appreciated!

Re: Circleci failures; looks like it's just some tox issues. You can run that via tox -v --recreate, making sure to pip install -r ./test-requirements.txt so you have tox.

I think it's just a line that's too long:

tests/test_utils.py:59:80: E501 line too long (85 > 79 characters)
1       E501 line too long (85 > 79 characters)

mzsanford commented 7 years ago

I fixed the PEP8 errors but Circle CI is still saying

Traceback (most recent call last):
  File "/home/ubuntu/airbrake-python/tests/test_utils.py", line 47, in test_get_local_git_revision
    self.assertIsNotNone(rev_file)
AssertionError: unexpectedly None

… which makes me thing the Circle CI environment differs in some way from a local git checkout. I went ahead and added a message to that assertion and now I see:

Traceback (most recent call last):
  File "/home/ubuntu/airbrake-python/tests/test_utils.py", line 50, in test_get_local_git_revision
    ref_path))
AssertionError: No revision in /home/ubuntu/airbrake-python/.git/HEAD: ref: refs/heads/pull/83

I am assuming this has to do with the way Circle CI does checkouts for pull requests.

zachgoldstein commented 7 years ago

@mzsanford ah I see. The problem here is that our circleci tests do not pull anything into ./.git/refs/heads/pull, so the test for that will fail. I was confused about why this was not an issue before, and digging into it, it seems like our assertion assertIsNotNone was getting passed 'None', which actually passes. So it was broken but not causing the test to fail. Your fix here exposed this, which is great.

@phumpal had a great suggestion to use $CIRCLE_SHA1. For now I think we should check this, and if it's set, skip the assertion self.assertIsNotNone(rev_file). We'll need to find a way to solve this more long-term on circleci, I imagine there's some configuration to pull in these git ref files. For now though, just skip it and I'll go in later for a more proper fix.

Thanks again for the effort here!

airbrake / airbrake-python

Silence the stderr git warning when running from a non-git directory #83