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

prohibit pushes if nbstripout is not installed #101

Closed
robertour opened this issue Jul 5, 2019 · 7 comments
Closed

prohibit pushes if nbstripout is not installed #101

robertour opened this issue Jul 5, 2019 · 7 comments

Comments

@robertour
Copy link

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 (#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 kynan changed the title prohibit pushes if nbstripout is not install prohibit pushes if nbstripout is not installed Jul 7, 2019
@kynan kynan added state:waiting Waiting for response for reporter type:question labels Jul 7, 2019
@kynan
Copy link
Owner

kynan commented Jul 7, 2019

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

robertour commented Jul 8, 2019

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
Copy link
Owner

kynan commented Jul 15, 2019

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
Copy link
Owner

kynan commented Aug 11, 2019

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

@robertour
Copy link
Author

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

@kynan kynan added resolution:fixed and removed state:waiting Waiting for response for reporter labels Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants