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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize git hooks #11442

Merged
merged 22 commits into from
Sep 2, 2021
Merged

Reorganize git hooks #11442

merged 22 commits into from
Sep 2, 2021

Conversation

kir0ul
Copy link
Contributor

@kir0ul kir0ul commented Jul 23, 2021

@bryevdv
Copy link
Member

bryevdv commented Jul 23, 2021

@kir0ul the full codebase test takes quite awhile to run, in the past it was set up as pre-push, not pre-commit I think it is possible to configure that with the pre-commit tool (despite the name)? we also need to add a custom pre-push hook that asks a confirmation before pushing to main or branch-*

Edit: I guess I was mistaken, it seems the dev docs have this as a pre-commit hook. @tcmetzger do you have thoughts? Should we keep it as pre-commit or move to pre-push? pre-commit is better in an "ideal" sense but it does take awhile to run so might be annoying on every commit.

@kir0ul
Copy link
Contributor Author

kir0ul commented Jul 23, 2021

Didn't try it but I think changing the default stages like below should do the trick:

stages: [push]

@bryevdv
Copy link
Member

bryevdv commented Jul 23, 2021

This is the old branch check hook, FYI

#!/bin/bash

protected_branch='master'
current_branch=$(git symbolic-ref HEAD | sed -e 's,.*/\(.*\),\1,')

if [ $protected_branch = $current_branch ]
then
    read -p "You're about to push master, is that what you intended? [y|n] " -n 1 -r < /dev/tty
    echo
    if echo $REPLY | grep -E '^[Yy]$' > /dev/null
    then
        exit 0 # push will execute
    fi
    exit 1 # push will not execute
else
    exit 0 # push will execute
fi

the protected branches today should be main and branch-* (and as you mention it should be converted to python)

@tcmetzger
Copy link
Member

Running the full codebase test does take a while (around 35s on my local Ubuntu system, almost twice as long on win10), so it might be worth looking into transferring that into a pre-push. Would it make sense to run a reduced set for pre-commit, e.g. only include flake8 and isort? In fact, eslint seems to take up the most time in my case...

@bryevdv
Copy link
Member

bryevdv commented Jul 23, 2021

Would it make sense to run a reduced set for pre-commit

yes I think that's a reasonable compromise

@kir0ul
Copy link
Contributor Author

kir0ul commented Jul 24, 2021

So now it works only on pre-push but it has to be specifically installed with pre-commit install --hook-type pre-push.

@bryevdv
Copy link
Member

bryevdv commented Jul 24, 2021

@kir0ul is it possible to combine multiple hook types in a single invocation? That would not be so bad, though I am really surprised there is not a "install all hook types" option.

@kir0ul
Copy link
Contributor Author

kir0ul commented Jul 26, 2021

@kir0ul is it possible to combine multiple hook types in a single invocation? That would not be so bad, though I am really surprised there is not a "install all hook types" option.

It seems there's no way to install all hooks, you need to specify each type of hook you want to install (or uninstall).
Some references:

@bryevdv
Copy link
Member

bryevdv commented Jul 26, 2021

@kir0ul that first link seems to indicate that this usage will work:

pre-commit install --install-hooks --hook-type pre-commit --hook-type commit-msg

i.e. providing multiple --install-hooks will work. It's a bit clunky but I think we can just provide the exact command in the docs for users to copy and paste

Edit: Also if it is too ugly/complicated, we could also make a python script in the scripts directory that just uses subprocess.run to run the actual command, e.g scripts/install-hooks.py, and tell users to run that.

@kir0ul
Copy link
Contributor Author

kir0ul commented Jul 26, 2021

For now there's only the pre-push in the docs, which seems easy enough to copy/paste: https://github.com/bokeh/bokeh/pull/11442/files#diff-ff1c6104bf973af764540cd9e9f6f048cc915bec9cfa56321e87c2a869a9f16bR168

@bryevdv
Copy link
Member

bryevdv commented Jul 26, 2021

@kir0ul I do think we want to add a quick pre-commit that runs a subset of the codebase tests (flake8, isort, and the quick "codebase checks" test) so I think a scripts/install-hooks.py is maybe the best option. It's also "future proof" in the sense that users can always run the same script, even if we add more or different hooks in the future (so less docs to maintain)

@bryevdv
Copy link
Member

bryevdv commented Aug 17, 2021

@kir0ul Just checking in do you plan to continue work on this PR?

@kir0ul
Copy link
Contributor Author

kir0ul commented Aug 17, 2021

Yes! But probably not before next week if that's ok.

@kir0ul kir0ul marked this pull request as ready for review August 24, 2021 23:57
scripts/protect_branch_git_hook.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
environment.yml Show resolved Hide resolved
scripts/install-hooks.py Outdated Show resolved Hide resolved
scripts/protect_branch_git_hook.py Outdated Show resolved Hide resolved
sphinx/source/docs/dev_guide/setup.rst Outdated Show resolved Hide resolved
sphinx/source/docs/dev_guide/setup.rst Outdated Show resolved Hide resolved
@bryevdv
Copy link
Member

bryevdv commented Aug 25, 2021

Should I test those 2 scripts?

We have very few scripts, and do not typically test them except manually

Should I also add a function to uninstall the hooks?

If it is trivial, then I guess it couldn't hurt, but I also don't think it is a priority. If you do add, let's put them in the hooks subdir I mentioned above as hooks/install.py and hooks/uninstall.py

Should I specify in the docs how to bypass the hooks in case?

cc @tcmetzger for thoughts, my inclination is to say no.

@kir0ul
Copy link
Contributor Author

kir0ul commented Aug 25, 2021

  • In addition to the codebase tests running before pushing, would it also make sense to add a hook that only run test_flake8.py, test_isort.py and test_code_quality.py before commiting?
  • About the uninstalling the hooks, another solution could be to put the hooks installation and uninstallation in the same file and pass some arguments like --install or --uninstall either directly to the script or by using click.

@kir0ul kir0ul marked this pull request as draft August 25, 2021 19:46
kir0ul and others added 5 commits August 25, 2021 21:48
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
@bryevdv
Copy link
Member

bryevdv commented Aug 26, 2021

In addition to the codebase tests running before pushing, would it also make sense to add a hook that only run test_flake8.py, test_isort.py and test_code_quality.py before commiting?

Opinions about pre-commit checks seem to vary, let's wait on this.

About the uninstalling the hooks, another solution could be to put the hooks installation and uninstallation in the same file and pass some arguments like --install or --uninstall either directly to the script or by using ​click.

We don't currently rely on click anywhere else, so I woudln't want to take on a new dev dependency just for this.
(Someday I might like to replace argparse with click in bokeh serve) Let's just have two scripts under a hooks subdir.

@tcmetzger
Copy link
Member

Should I specify in the docs how to bypass the hooks in case?

cc @tcmetzger for thoughts, my inclination is to say no.

I agree with @bryevdv that we should keep the docs as simple as possible. Mentioning pre-commit install and pre-commit uninstall (and maybe pre-commit run [path]) should be sufficient. Beyond that, I'd link to the official pre-commit docs.

@tcmetzger
Copy link
Member

tcmetzger commented Aug 26, 2021

By the way, this PR looks really great! git-commit will make things a lot easier for contributors, I'm looking forward to using it! 😀

@kir0ul kir0ul marked this pull request as ready for review September 2, 2021 00:44
environment.yml Outdated Show resolved Hide resolved
@bryevdv bryevdv added this to the 2.4 milestone Sep 2, 2021
@bryevdv bryevdv merged commit 2c659ea into bokeh:branch-2.4 Sep 2, 2021
@bryevdv
Copy link
Member

bryevdv commented Sep 2, 2021

Thanks for the PR @kir0ul and thanks for your patience in pushing it through!

bryevdv added a commit that referenced this pull request Dec 13, 2021
* feat: run codebase tests on pre-commit

* Try pre-push

* Try pre-push (bis)

* docs: updated Git Hooks docs

* docs: fix "code-block" directive arguments

* feat: add git hook to protect master branch

* ci: add pre-commit to CI env files

* refactor: comments from review

* refactor: remove master

* chore: annotate function with type

* feat: add wrapper script to install hooks

* refactor: subprocess command

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* refactor: long string

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* refactor: RST syntax

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* docs: improve Git hooks description

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* ci: fix trailing whitespace

* refactor: move in hooks folder

* feat: add uninstall script for pre-push hooks

* fix: style related comment

Related to #10712

* refactor: rename git hook

* fix: isort

* fix: remove optional document start marker in YAML

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants