Skip to content
This repository has been archived by the owner on Jan 31, 2018. It is now read-only.

[bug 1133121] Use Yelp's pre-commit to provide pre-commit hooks #547

Merged
merged 1 commit into from Apr 21, 2015

Conversation

lgp171188
Copy link
Contributor

Hook configuration in .pre-commit-config.yaml based on those of richard and kitsune. flake8 exclude directories and arguments taken from existing flake8 lint script. Removed the existing pre-commit hook scripts.

@willkg
Copy link
Member

willkg commented Apr 20, 2015

Awesome! I'll check this out today and test it in vagrant, too.

@lgp171188
Copy link
Contributor Author

pre-commit jshint hook sets up its own environment where it installs jshint and uses it. Doesn't use the version we have installed. So I have removed it from our packages.json. Also the installation of pre-commit's jshint takes a long time and often gets killed due to the low free memory in the default configuration of the VM which is 512 MB as of now. I had to increase it to 1 GB to get it working.

I will be working next on the elasticsearch resource utilization optimization and I will increase the VM's RAM. So I guess this can wait till that is merged.

@willkg
Copy link
Member

willkg commented Apr 20, 2015

Oh, interesting. Ok--let's wait on merging this for now, then.

@willkg
Copy link
Member

willkg commented Apr 20, 2015

I talked with @mythmon about this and SUMO. I'm not concerned with landing this anymore. I think we should just do it.

The memory issue is an issue. I think we should just land that without the rest of the ES fixes.

I'll test this out now.

virtualenv==12.1.1

# sha256: PkWmgeXnnrMfCUsxhDNG7Zo95Of2682hDHSLPEXFWX4
simplejson==3.0.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting this error after installing everything in requirements/dev.txt:

 <fjord> (<detached:remotes/official/pr/547=105a3 fjord/ scripts/) saturn2 ~/mozilla/fjord> pre-commit install
Traceback (most recent call last):
  File "/home/willkg/.virtualenvs/fjord/bin/pre-commit", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 2829, in <module>
    working_set = WorkingSet._build_master()
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 449, in _build_master
    ws.require(__requires__)
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 742, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 639, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: ordereddict

If I install ordereddict, then I hit this error:

<fjord> (<detached:remotes/official/pr/547=105a3 fjord/ scripts/) saturn2 ~/mozilla/fjord> pre-commit install
Traceback (most recent call last):
  File "/home/willkg/.virtualenvs/fjord/bin/pre-commit", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 2829, in <module>
    working_set = WorkingSet._build_master()
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 449, in _build_master
    ws.require(__requires__)
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 742, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/home/willkg/.virtualenvs/fjord/lib/python2.7/site-packages/pkg_resources.py", line 639, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: argparse

If I install argparse, then things work.

So I think we need to add:

argparse==1.3.0
ordereddict==1.1

plus their SHA hashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure I searched for ordereddict on pypi and added a version line for it. Don't know what happened after that and how I missed that :) Maybe I did something in my sleepiness, will fix this and update the pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the pull request with those packages. I am still wondering how it got through considering the pre-commit hooks got installed and worked fine for me :-S

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally fine! You might have had them in your environment already or something. This is exactly what peer review is for. I do this sort of thing all the time, though it's usually in the form of missing files.

@willkg
Copy link
Member

willkg commented Apr 21, 2015

This is going to be easier to test live. Given that it affects developer environments and not the production servers, I'm just going to land it and test it live. We'll fix anything that pops up in future PRs.

Thank you!

willkg added a commit that referenced this pull request Apr 21, 2015
[bug 1133121] Use Yelp's pre-commit to provide pre-commit hooks
@willkg willkg merged commit 29b0d19 into mozilla:master Apr 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants