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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Overcommit #765

Merged
merged 5 commits into from Feb 22, 2017
Merged

Add Overcommit #765

merged 5 commits into from Feb 22, 2017

Conversation

markolson
Copy link
Contributor

@markolson markolson commented Feb 8, 2017

Overcommit will run checks using git hooks against any staged code and commit messages before allowing the code to be committed or pushed. This example .overcommit.yml file follows what I've tended to use on past projects.

It's opt-in by running the overcommit --install --sign command, and can always be disabled or overridden if needed. Any changes made to the .overcommit.yml file or any plugins require overcommit --sign to be run again.

For an example of how it works, make one of the yaml configuration files invalid and try to commit it.

馃毑 馃彔

Overcommit will run checks against any staged code and commit messages
before allowing the code to be committed or pushed. This example
.overcommit.yml file follows what I've tended to use on past projects.

It's opt-in by running the `overcommit --install` command, and can
always be disabled or overridden if needed.

For an example of how it works, make one of the yaml configuration
files invalid and try to commit it.
Copy link
Contributor

@robbiethegeek-usds robbiethegeek-usds left a comment

Choose a reason for hiding this comment

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

Not sure that this should be optional for local development... @aub thoughts? @jkassemi (for vision)

@jkassemi
Copy link
Contributor

jkassemi commented Feb 8, 2017

I'll speak to CI integration and not to the update to developer workflow. There's some functionality overlap with rubocop (newline detection), but we can throw this into the Jenkinsfile (or as part of the ci rake task).

@markolson
Copy link
Contributor Author

I'm not entirely sure about the benefit of adding this to Jenkins, especially given the need to sign the .overcommit file whenever it changes. We'd have to run overcommit -r, which scans the entire repo instead of the just staged commits and could catch a lot more than is strictly necessary for the PR/branch itself.

@aub
Copy link
Contributor

aub commented Feb 17, 2017

@markolson is it even possible to make it non-optional? I think it looks good but might change the language in the README to something that makes it seem more imperative, or at least gives a reason why one should install it.

@markolson
Copy link
Contributor Author

Can do. I'll also change bin/setup as part of this to do a lot of what the README suggests, including enabling overcommit.

@aub
Copy link
Contributor

aub commented Feb 17, 2017

@markolson that sounds perfect!

@markolson
Copy link
Contributor Author

@aub I've added a (very likely overly complex) setup script for people using OS X which enables overcommit as part of the setup process.

Copy link
Contributor

@aub aub left a comment

Choose a reason for hiding this comment

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

Ok, this looks great. One last thing... does the README contain instructions to run your setup script? I think we're good to merge if we have that.

@markolson
Copy link
Contributor Author

@aub Updated the README. I also did a run-through where I deleted my homebrew, rubies, postgres, and redis and the script got me setup and running.

@aub
Copy link
Contributor

aub commented Feb 22, 2017

!!! Awesome, let's do this.

@aub aub merged commit 70522e5 into master Feb 22, 2017
@aub aub deleted the overcommit branch February 22, 2017 18:28
@aub aub removed the in progress label Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants