toplenboren
[feat] Add a new option to skip enabling on CI
#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.
@toplenboren I'd like to reuse SKIP_SIMPLE_GIT_HOOKS environment variable, how do you think?
@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.
Yes, probably. But are there instances where you'd want the hooks to be installed in a CI environment?
It's not about me personally, it's a behavior change which is a BREAKING CHANGE.
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.
I'm fine with BREAKING CHANGE or major version personally, let's wait for @toplenboren to confirm.
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
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…
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
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
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
@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.
Let's add SKIP_INSTALL_SIMPLE_GIT_HOOKS env variable support, and add CI variable support
- 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
- SKIP_INSTALL_SIMPLE_GIT_HOOKS=1
- Install hooks
- SKIP_INSTALL_SIMPLE_GIT_HOOKS=0
- do NOT install hooks
This way we will:
- have the env var namings consistent:
SKIP_* - be able to address each case
- not need to do a major release
What do you think, @JounQin. Will this cover your usecases?
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.
That‘s how I meant it: using is-ci should probably be outside of this library‘s scope, since it is easy enough.
That‘s how I meant it: using
is-cishould 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
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.
Oh, yes, I also think we should not check about CI on this lib's side. Do you have the consensus here?
I honestly really like this idea:
@JounQin Many CI environments normally set the
CIvariable 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
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?
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