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

[JounQin]

As title

[toplenboren]

Hi! I think this is going to be implemented once #96 make it to npm

[JounQin]

#96 is a bit different for skipping on committing, while this issue aims skipping installation at the beginning.

I'd like to work on a PR soon.

[JounQin]

@toplenboren I'd like to reuse SKIP_SIMPLE_GIT_HOOKS environment variable, how do you think?

[peschee]

@JounQin Many CI environments normally set the CI variable by default (https://docs.travis-ci.com/user/environment-variables/#default-environment-variables, https://docs.travis-ci.com/user/environment-variables/#default-environment-variables, https://docs.github.com/en/actions/learn-github-actions/variables), so it would be best to also support this by default.

[JounQin]

@peschee I think that would be a BREAKING CHANGE?

[peschee]

Yes, probably. But are there instances where you'd want the hooks to be installed in a CI environment?

[JounQin]

It's not about me personally, it's a behavior change which is a BREAKING CHANGE.

[peschee]

Yes, I understand. Breaking changes can happen though, so I'd be inclined to still vote for handling CI env vars, since this is what many other tools do.

[JounQin]

I'm fine with BREAKING CHANGE or major version personally, let's wait for @toplenboren to confirm.

[toplenboren]

Why do we need any git hooks to be set in CI ?

Maybe we can just add the guard to postinstall script, so that it does not install git hooks if it is executed inside CI environment

[peschee]

Husky uses a custom env var for this: https://typicode.github.io/husky/how-to.html#ci-server-and-docker

Maybe it is easier to off-load the responsibility of skipping to the user all together. Using something like is-ci is pretty straightforward…

[JounQin]

Why do we need any git hooks to be set in CI?

Checking commit message on CI for example.

Maybe we can just add the guard to postinstall script, so that it does not install git hooks if it is executed inside CI environment

I'd prefer to check SKIP_SIMPLE_GIT_HOOKS environment variable personally.

Using something like is-ci is pretty straightforward…

simple-git-hooks does have any dependencies to keep it simple as husky.

[toplenboren]

By the way, what is your usecase @JounQin?

[JounQin]

@toplenboren

In my company, we have components which checks previous commit message on CI.

While in some other components, we don't want to enable git hooks because we could commit automatically with other tools.

[toplenboren]

@toplenboren

In my company, we have components which checks previous commit message on CI.

While in some other components, we don't want to enable git hooks because we could commit automatically with other tools.

Thank you for clarification!

Why simply using SKIP_SIMPLE_GIT_HOOKS would not work? Hooks will be installed, but not executed

[JounQin]

@toplenboren

That's the point, we don't want the hooks to be installed and logs echo "[INFO] SKIP_SIMPLE_GIT_HOOKS is set to 1, skipping hook." on every commit.

[toplenboren]

Let's add SKIP_INSTALL_SIMPLE_GIT_HOOKS env variable support, and add CI variable support

1. Case: SKIP_INSTALL_SIMPLE_GIT_HOOKS is not set

• If not set and if in CI -> do NOT install hooks
• If not set and not in CI -> install hooks

2. SKIP_INSTALL_SIMPLE_GIT_HOOKS=1

• Install hooks

3. SKIP_INSTALL_SIMPLE_GIT_HOOKS=0

• do NOT install hooks

This way we will:

1. have the env var namings consistent: SKIP_*
2. be able to address each case
3. not need to do a major release

What do you think, @JounQin. Will this cover your usecases?

[JounQin]

Do you want to add a dependency like is-ci? I think it's out scope of this tool, the user should easily add an environment variable on their needs.

[peschee]

That‘s how I meant it: using is-ci should probably be outside of this library‘s scope, since it is easy enough.

[JounQin]

That‘s how I meant it: using is-ci should probably be outside of this library‘s scope, since it is easy enough.

Checking isCI properly is not that simple AFAIK.

https://unpkg.com/browse/[email protected]/index.js

[peschee]

It seems we're talking about two different things…

The way I meant it is what is in the husky docs. They DO NOT include the is-ci package in husky, but rather rely on the user controlling whether they call husky install or not (using is-ci or any whatever other method).

My point was: this library is probably better off not including an additional dependency.

[JounQin]

Oh, yes, I also think we should not check about CI on this lib's side. Do you have the consensus here?

[toplenboren]

I honestly really like this idea:

@JounQin Many CI environments normally set the CI variable by default (https://docs.travis-ci.com/user/environment-variables/#default-environment-variables, https://docs.travis-ci.com/user/environment-variables/#default-environment-variables, https://docs.github.com/en/actions/learn-github-actions/variables), so it would be best to also support this by default.

Let's just check CI variable and add INSTALL_SIMPLE_GITH_HOOKS to explicitly configure this package for narrow usecases

[JounQin]

Let's just check CI variable and add INSTALL_SIMPLE_GITH_HOOKS to explicitly configure this package for narrow usecases

OK, I'm personally fine with this strategy.

---

Case: SKIP_INSTALL_SIMPLE_GIT_HOOKS is not set

• If not set and if in CI -> do NOT install hooks
• If not set and not in CI -> install hooks

not need to do a major release

Isn't it a BREAKING CHANGE for not installing on CI by default?

[toplenboren]

Let's just check CI variable and add INSTALL_SIMPLE_GITH_HOOKS to explicitly configure this package for narrow usecases

OK, I'm personally fine with this strategy.

Case: SKIP_INSTALL_SIMPLE_GIT_HOOKS is not set

• If not set and if in CI -> do NOT install hooks
• If not set and not in CI -> install hooks

not need to do a major release

Isn't it a BREAKING CHANGE for not installing on CI by default?

Well.. technically yes.. :( I think we can do a major release, no problem

We can also split: first add the ability to configure INSTALL_SIMPLE_GIT_HOOKS and then add CI processing and update major

[JounQin]

I'll work on it soon.