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

Remove husky #24887

Merged
merged 7 commits into from
Nov 15, 2018
Merged

Remove husky #24887

merged 7 commits into from
Nov 15, 2018

Conversation

mistic
Copy link
Member

@mistic mistic commented Oct 31, 2018

This PR closes the issue #19576
It basically removes the husky 3rd party dependency and provides a simple way to register a git hook script for the pre-commit.

@mistic mistic added review Team:Operations Team label for Operations Team v7.0.0 v6.6.0 labels Oct 31, 2018
@mistic mistic self-assigned this Oct 31, 2018
@mistic mistic requested review from tylersmalley and jbudz October 31, 2018 04:36
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Defensive and clear 👍

@tylersmalley
Copy link
Contributor

tylersmalley commented Nov 2, 2018

For reference, here is the previous pre-commit hook:

#!/bin/sh
# husky
cd .
npm run --json | grep -q '"precommit":'
if [ $? -ne 0 ]; then
  exit 0
fi

npm run precommit --silent

if [ $? -ne 0 ]; then
  echo
  echo "husky - pre-commit hook failed (add --no-verify to bypass)"
  echo
  exit 1
fi

@mistic
Copy link
Member Author

mistic commented Nov 12, 2018

@tylersmalley I just have applied every change we talked about! Do you mind take a look to it again?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mistic
Copy link
Member Author

mistic commented Nov 14, 2018

@tylersmalley I just applied every change you suggest! Thanks for the feedback 😃

@elastic elastic deleted a comment from elasticmachine Nov 14, 2018
@mistic
Copy link
Member Author

mistic commented Nov 14, 2018

retest

1 similar comment
@tylersmalley
Copy link
Contributor

retest

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM

@tylersmalley
Copy link
Contributor

Ah, you will need to update this branch from master.

@elastic elastic deleted a comment from elasticmachine Nov 14, 2018
@elastic elastic deleted a comment from elasticmachine Nov 14, 2018
@mistic
Copy link
Member Author

mistic commented Nov 14, 2018

yeah, done!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mistic mistic merged commit 1155b81 into elastic:master Nov 15, 2018
mistic added a commit to mistic/kibana that referenced this pull request Nov 15, 2018
* feat(NA): remove husky dependency and implement a git hooks registering mechanism.

* fix(NA): apply executable chmod to the created kbn git hook script.

* fix(NA): run npm script on the git pre commit hook silently.

* refact(NA): apply changes according the PR review.

* refact(NA): apply changes according PR review.
mistic added a commit that referenced this pull request Nov 15, 2018
* feat(NA): remove husky dependency and implement a git hooks registering mechanism.

* fix(NA): apply executable chmod to the created kbn git hook script.

* fix(NA): run npm script on the git pre commit hook silently.

* refact(NA): apply changes according the PR review.

* refact(NA): apply changes according PR review.
@mistic
Copy link
Member Author

mistic commented Nov 15, 2018

6.x: 4b31c9b

@weltenwort
Copy link
Member

Unfortunately this does not seem to be compatible with the git worktree feature. Other than husky, this fails with

Registering Kibana pre-commit git hook...

fail Kibana pre-commit git hook was not installed as an error occur.

ERROR Error: ENOTDIR: not a directory, open '${WORKTREE_DIR}/.git/hooks/pre-commit'
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

    at makeError (${WORKTREE_DIR}/packages/kbn-pm/dist/index.js:19977:9)
    at Promise.all.then.arr (${WORKTREE_DIR}/packages/kbn-pm/dist/index.js:20082:16)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
error Command failed with exit code 1.

because it mistakenly assumes ${WORKTREE_DIR}/.git to be a directory instead of a file as is the case in a worktree.

@weltenwort
Copy link
Member

weltenwort commented Nov 19, 2018

@mistic
Copy link
Member Author

mistic commented Nov 19, 2018

@weltenwort thanks for letting us know about it!

I think maybe we can use git rev-parse --git-dir to reliable get the .git dir location. Is this actually working with your use case for git worktree?

@weltenwort
Copy link
Member

git rev-parse --git-dir seems to work, but we could also just learn from husky and spare the subprocess call 😉 for now I'm manually fixing the REPO_ROOT in src/dev/constants.ts.

@mistic
Copy link
Member Author

mistic commented Nov 19, 2018

@weltenwort can you please take a look on #25870 as you were the first reporting this bug? 😃 Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants