feat: add env var to bypass hooks execution #96

bhanufyi · 2023-09-04T17:43:33Z

ref -
#87

bhanufyi · 2023-09-09T07:08:46Z

@toplenboren can you please have a look at this, when you have time. Thank you 😄

toplenboren

Hi! thanks for the PR! Looks good, but have a few points that can be polished :-)

README.md

simple-git-hooks.js

simple-git-hooks.test.js

toplenboren · 2023-09-30T19:32:28Z

README.md

@@ -158,6 +158,18 @@ You should use `--no-verify` option

 https://bobbyhadz.com/blog/git-commit-skip-hooks#skip-git-commit-hooks

+If you need to bypass hooks for multiple Git operations, setting the SKIP_SIMPLE_GIT_HOOKS environment variable can be more convenient. Once set, all subsequent Git operations in the same terminal session will bypass the associated hooks.


It's best to also include use cases. I've seen people struggle with skipping git hooks when using 3rd party git clients

Normally (from the terminal) you can bypass hooks by using --skip-hooks option if im not mistaken :-)

Like so:

Using with 3rd party clients:

...

toplenboren · 2023-09-30T19:48:42Z

simple-git-hooks.test.js

-    expect(JSON.stringify(installedHooks)).toBe(JSON.stringify({'pre-commit':`#!/bin/sh\nexit 1`, 'pre-push':`#!/bin/sh\nexit 1`}))
-
-    removeGitHooksFolder(projectWithConfigurationInSeparateJsPath)
+    expect(JSON.stringify(installedHooks)).toBe(JSON.stringify({'pre-commit':`${spc.PREPEND_SCRIPT}exit 1`, 'pre-push':`${spc.PREPEND_SCRIPT}exit 1`}))


I think, its best to move this to separate variables, so the code not repeated:

Im thinking of something like this: wyt?

const TEST_SCRIPT = ${spc.PREPEND_SCRIPT}${exit 1}`

Then we can use it in tests like so:

expect(JSON.stringify(installedHooks)).toBe(JSON.stringify({'pre-commit':``${TEST_SCRIPT}, 'pre-push':${spc.TEST_SCRIPT}}))

you can also try to use loadash compare functions, they can compare two objects deeply, so we won't need to use STRINGIFY hack

used lodash.isequal package for this

toplenboren · 2023-09-30T20:10:16Z

simple-git-hooks.test.js

 })
+
+
+test("bypasses hooks when SKIP_SIMPLE_GIT_HOOKS is set to 1", () => {


You can also try grouping these test cases using describe syntax. It it a good practice

toplenboren · 2023-09-30T20:10:51Z

simple-git-hooks.test.js

+   createGitHooksFolder(projectWithConfigurationInPackageJsonPath);
+
+   spc.setHooksFromConfig(projectWithConfigurationInPackageJsonPath);
+   process.env.SKIP_SIMPLE_GIT_HOOKS = '1';  // Set environment variable


I think you might need to unset variable at the end of test case.. Maybe move to after Each ? Not sure

yes, I was clearing it in global afterEach, now moved to local afterEach in describe simple git hooks env var tests

toplenboren · 2023-10-07T18:24:17Z

simple-git-hooks.test.js

-    expect(JSON.stringify(installedHooks)).toBe(JSON.stringify({'pre-commit':`#!/bin/sh\nexit 1`, 'pre-push':`#!/bin/sh\nexit 1`}))
-
-    removeGitHooksFolder(projectWithConfigurationInSeparateCjsPath)
+    expect(isEqual(installedHooks, { 'pre-commit': TEST_SCRIPT, 'pre-push': TEST_SCRIPT })).toBe(true);


I think we can extract { 'pre-commit': TEST_SCRIPT, 'pre-push': TEST_SCRIPT } as variable too

yes, extracted into COMMON_GIT_HOOKS variable

toplenboren · 2023-10-07T18:34:41Z

simple-git-hooks.test.js

 })
+
+describe('Tests for skipping git hooks using SKIP_SIMPLE_GIT_HOOKS env var', () => {


It's nice to see you taking a look to jest describe syntax here!

But it makes no sense if it is used only in your tests. Let me try to be more clear on what I meant in the previous review:

So, we have our test code setup like this:

// Get project root directory test('getProjectRootDirectory returns correct dir in typical case:', () => { expect(spc.getProjectRootDirectoryFromNodeModules('var/my-project/node_modules/simple-git-hooks')).toBe('var/my-project') }) test('getProjectRootDirectory returns correct dir when used with windows delimiters:', () => { expect(spc.getProjectRootDirectoryFromNodeModules('user\\allProjects\\project\\node_modules\\simple-git-hooks')).toBe('user/allProjects/project') })

We can use describe to make it look like this:

describe('Get Project Root Directory tests', () => { it('returns correct dir in typical case:', () => { expect(spc.getProjectRootDirectoryFromNodeModules('var/my-project/node_modules/simple-git-hooks')).toBe('var/my-project') }) it('returns correct dir when used with windows delimiters:', () => { expect(spc.getProjectRootDirectoryFromNodeModules('user\\allProjects\\project\\node_modules\\simple-git-hooks')).toBe('user/allProjects/project') }) })

This way tests can be grouped, which allows us to see pretty output: here is an example if we do the manipulations above

--

I propose two solutions:

refactor all the tests to use describe syntax

or simply leave your tests as is (do not use describe syntax) - I will do refactoring later :-)

I have used describe, it syntax, let me if it needs further work

toplenboren · 2023-10-07T18:36:13Z

simple-git-hooks.test.js

+
+describe('Tests for skipping git hooks using SKIP_SIMPLE_GIT_HOOKS env var', () => {
+
+    const GIT_USER_NAME = "github-actions";


I think we can do this in GitHub actions itself, do not see why we need some GH actions specific code in repo

In the tests, I'm performing commit git operation which requires to set username and email, so I have used random values, let me know if it can be done in a better way

bhanufyi · 2023-11-03T16:38:07Z

@toplenboren added the improvements that you have suggested, can you please have a look when you get some time. Thank you

bhanufyi · 2023-11-27T06:01:28Z

Hi @toplenboren, pinging to see if we can merge this pr, or need anymore changes

toplenboren · 2023-11-30T20:36:43Z

Hi, I think I already approved it last time, but will review your PR again this weekends

Thanks

bhanufyi · 2024-01-03T05:10:14Z

@toplenboren any expected timeline for merging this PR ?

toplenboren · 2024-01-03T11:49:07Z

@toplenboren any expected timeline for merging this PR ?

Yep. gonna do it this week

jochemkeller · 2024-02-20T15:49:54Z

Hi @toplenboren, thanks for merging the PR and many thanks to @bhanuprasadcherukuvada for adding the feature 🙏 it's very handy for my current use-case!

Can I kindly request that this feature be made available through a new NPM version? After some debugging I found out that this feature isn't available in the current latest version of simple-git-hooks, even though the GitHub master README documentation shows that the feature should be available 🤪 that was a confusing 15 minutes there..

JounQin · 2024-02-25T17:15:55Z

package.json

@@ -44,6 +44,7 @@
    "clean-publish": "^4.2.0",
    "eslint": "^7.19.0",
    "jest": "^26.6.3",
-    "lint-staged": "^10.5.4"
+    "lint-staged": "^10.5.4",
+    "lodash.isequal": "^4.5.0"


What is this? I saw no usage?

bhanufyi added 2 commits September 4, 2023 14:03

feat: add env var to bypass hooks execution

79136a8

fix ci git init issues

3d0ff4c

toplenboren reviewed Sep 24, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

simple-git-hooks.js Outdated Show resolved Hide resolved

simple-git-hooks.test.js Outdated Show resolved Hide resolved

bhanufyi added 3 commits September 25, 2023 00:34

fix(tests): replace with single afterEach

c437eb7

feat: export prepend script, reuse in tests

10f1ded

feat: use SKIP_SIMPLE_GIT_HOOKS=1 for bypassing

6ad9fe4

bhanufyi requested a review from toplenboren September 29, 2023 17:47

toplenboren reviewed Sep 30, 2023

View reviewed changes

bhanufyi added 4 commits October 7, 2023 02:51

feat: use describe, reduce code duplication

062a539

feat: add lodash.isequal and use isEqual instead of json stringify

d0b35e1

refactor: improve tests by reducing duplication

b506dbd

docs: mention about 3rd party git clients

095089d

bhanufyi requested a review from toplenboren October 6, 2023 22:44

toplenboren approved these changes Oct 7, 2023

View reviewed changes

refactor: use describe, it syntax for test

47dec20

bhanufyi requested a review from toplenboren October 20, 2023 18:37

toplenboren approved these changes Dec 2, 2023

View reviewed changes

toplenboren merged commit 20121df into toplenboren:master Jan 4, 2024
4 checks passed

jochemkeller mentioned this pull request Feb 20, 2024

Skip hooks installation when env variable is set #87

Closed

toplenboren mentioned this pull request Feb 25, 2024

[feat] Add a new option to skip enabling on CI #99

Closed

JounQin reviewed Feb 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add env var to bypass hooks execution #96

feat: add env var to bypass hooks execution #96

bhanufyi commented Sep 4, 2023

bhanufyi commented Sep 9, 2023

toplenboren left a comment

toplenboren Sep 30, 2023

toplenboren Sep 30, 2023

toplenboren Sep 30, 2023

bhanufyi Oct 6, 2023

toplenboren Sep 30, 2023

toplenboren Sep 30, 2023

bhanufyi Oct 6, 2023

toplenboren Oct 7, 2023

bhanufyi Oct 20, 2023

toplenboren Oct 7, 2023

bhanufyi Oct 20, 2023

toplenboren Oct 7, 2023

bhanufyi Oct 20, 2023

bhanufyi commented Nov 3, 2023

bhanufyi commented Nov 27, 2023

toplenboren commented Nov 30, 2023

bhanufyi commented Jan 3, 2024

toplenboren commented Jan 3, 2024 •

edited

Loading

jochemkeller commented Feb 20, 2024

JounQin Feb 25, 2024

		@@ -158,6 +158,18 @@ You should use `--no-verify` option

		https://bobbyhadz.com/blog/git-commit-skip-hooks#skip-git-commit-hooks

		If you need to bypass hooks for multiple Git operations, setting the SKIP_SIMPLE_GIT_HOOKS environment variable can be more convenient. Once set, all subsequent Git operations in the same terminal session will bypass the associated hooks.

		})


		test("bypasses hooks when SKIP_SIMPLE_GIT_HOOKS is set to 1", () => {

		})

		describe('Tests for skipping git hooks using SKIP_SIMPLE_GIT_HOOKS env var', () => {


		describe('Tests for skipping git hooks using SKIP_SIMPLE_GIT_HOOKS env var', () => {

		const GIT_USER_NAME = "github-actions";

feat: add env var to bypass hooks execution #96

feat: add env var to bypass hooks execution #96

Conversation

bhanufyi commented Sep 4, 2023

bhanufyi commented Sep 9, 2023

toplenboren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Using with 3rd party clients:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhanufyi commented Nov 3, 2023

bhanufyi commented Nov 27, 2023

toplenboren commented Nov 30, 2023

bhanufyi commented Jan 3, 2024

toplenboren commented Jan 3, 2024 • edited Loading

jochemkeller commented Feb 20, 2024

Choose a reason for hiding this comment

toplenboren commented Jan 3, 2024 •

edited

Loading