kynan / nbstripout

strip output from Jupyter and IPython notebooks
Other
1.21k stars 96 forks source link

prohibit pushes if nbstripout is not installed #101

Closed robertour closed 5 years ago

robertour commented 5 years ago

Is there any way to configure github (with hooks maybe?) so that pushes are impossible when nbstripout is not installed.

I struggle with developers that forget to install nbstripout after uninstall it. Sometimes, we are forced to do uninstall because of this bug (https://github.com/kynan/nbstripout/issues/65); which seems to be very difficult to fix.

So, an alternative to avoid dirty jupyter notebooks would be to avoid pushes when nbstripout is not installed. I think hooks would be a way to go, but I have never used one.

kynan commented 5 years ago

I think what you want is a pre-commit hook to already prevent commits when nbstripout is not installed.

You could ask your developers to run the following in the root of their repositories (only tested on Linux):

cat > .git/hooks/pre-commit <<EOF
# If no .ipynb files are to be committed, do nothing.
git diff --cached --name-only --diff-filter=ACM | grep -q -v -e '\.ipynb$' && exit 0

exec 2>&1

if hash nbstripout 2>/dev/null; then
  # If nbstripout is installed, do nothing.
  nbstripout --is-install && exit 0
  echo "nbstripout is not installed in this repository"
  exit 1
else
  echo "nbstripout command was not found"
  exit 1
fi
EOF
chmod +x .git/hooks/pre-commit
robertour commented 5 years ago

Thanks! This almost fixed the issue, except that it won't block the developers to push changes withoug being the notebooks being cleaned. I didn't see the issue when I was posted the question.

So, the pre-commit hook is better the pre-push hook (if that even exist), but a pre-add hook would be perfect. The problem with the pre-commit is that developers are required to do a git reset HEAD to unstage the changes after the errors above are triggered. For example:

$nbstripout --uninstall
.$.. # solved the problems due to issue https://github.com/kynan/nbstripout/issues/65
$... # forgets to install nbstripout
$git add .
$git commit -m 'blah'
nbstripout is not installed in this repository
$nbstripout --install
$git commit -m 'blah'
$git push 

I re-read the documentation, and I though that the (manual-filter-installation)[https://github.com/kynan/nbstripout#manual-filter-installation] could help me there but I seem to be confusing what it dows.

I would also like to add this pre-commit or pre-add to the github repository, so it is activated by default after cloning.

robertour commented 5 years ago

Is there a way of making sure that the ipynb is clean ("nbstripped"). At least, I can prevent commits if this is not the case.

robertour commented 5 years ago

I modified the script.

  1. I fixed a bug in the first line, as it was failing when there was an .ipynb file and a non .ipynb file, due to the reverse paramater (-v). Instead I placed the negation outside the command ( with !)

  2. I applied a git reset after attempting a commit when the nbstripout is not installed.

# If no .ipynb files are to be committed, do nothing.
! git diff --cached --name-only --diff-filter=ACM | grep -q -e '\.ipynb$' && exit 0

exec 2>&1

if hash nbstripout 2>/dev/null; then
  # If nbstripout is installed, do nothing.
  nbstripout --is-install && exit 0
  git reset
  echo "nbstripout is not installed in this repository. Your changes have been unstaged"
  exit 1
else
  git reset
  echo "nbstripout command was not found. Your changes have been unstaged."
  exit 1
fi

However, this is not yet perfect. Somebody can still circumvent the nbstripout by installing nbstripout just before the commit.

$nbstripout --uninstall
$git add .
$nbstripout --install
$git commit -m 'blah'
$git push 

However, the above feels almost intentional.

Unfortunately, hooks are not designed to be part of the repository.

kynan commented 5 years ago

As you already found out there is no way to make git activate a hook after cloning the repository (this is by design to prevent malicious code from being executed).

I think you want to prevent accidental rather than intentional cases of nbstripout not being installed, so I don't the last point is much of a concern (also because you can simply run git commit --no-verify to skip the pre-commit hook).

There are 2 pull requests adding support for pre-commit.com, however their intent is different from yours I think.

kynan commented 5 years ago

@robertour friendly ping, have you been able to resolve this to your satisfaction?

robertour commented 5 years ago

I think so, it has done the trick.

I would just add that it is also a good idea to install this at a git global level. I share some instructions that I give to the programmers.

  1. Enable git templates:

git config --global init.templatedir '~/.git-templates'

This tells git to copy everything in ~/.git-templates to your per-project .git/ directory when you run git init

2.Create a directory to hold the global hooks:

mkdir -p ~/.git-templates/hooks

  1. Write the following hook in ~/.git-templates/hooks/pre-commit.
# If no .ipynb files are to be committed, do nothing.
! git diff --cached --name-only --diff-filter=ACM | grep -q -e '\.ipynb$' && exit 0

exec 2>&1

if hash nbstripout 2>/dev/null; then
  # If nbstripout is installed, do nothing.
  nbstripout --is-install && exit 0
  git reset
  echo "nbstripout is not installed in this repository. Your changes have been unstaged"
  exit 1
else
  git reset
  echo "nbstripout command was not found. Your changes have been unstaged."
  exit 1
fi
  1. Make sure the hook is executable.

chmod a+x ~/.git-templates/hooks/pre-commit

  1. Re-initialize git in each existing repo you’d like to use this in:

git init