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.

Sign up for GitHub

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

Jump to bottom

Proposal: remove automatic installation of pre-commit hooks on bootstrap #851

Open
kavilla opened this issue Oct 7, 2021 · 3 comments
Open

Proposal: remove automatic installation of pre-commit hooks on bootstrap #851

kavilla opened this issue Oct 7, 2021 · 3 comments
Labels
enhancement New feature or request technical debt If not paid, jeapardizes long-term success and maintainability of the repository.

Comments

@kavilla
Copy link
Member

kavilla commented Oct 7, 2021 •
edited
Loading

Is your feature request related to a problem? Please describe.

#850, came about because I was trying to bootstrap on an incompatible version of git. The bootstrap script automatically calls the script to install pre-commit git hooks, if the git version is not compatible then it would fail, I have made PR to fail silently on the error that stems from git version here: #852.

Describe the solution you'd like

Pre-commit git hooks shouldn't be a blocker when trying to just build the application. So it should by default not be installed for application. Developers can still call node scripts/register_git_hook or we can create another package.json script to call this script. But we should do in this a 2+ version of OpenSearch Dashboards so we don't break current workflows.
@kavilla kavilla added the enhancement New feature or request label Oct 7, 2021
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 7, 2021
@kavilla
[Build] fail silently on register git hook failure
b943712 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and if not $GIT_DIR. Then it proceeds to try to install from a
path that doesn't exist because it will use `--git-common-dir` in the
path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
opensearch-project#851

Issue resolved:
opensearch-project#850

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla mentioned this issue Oct 7, 2021
Merged
5 tasks
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 7, 2021
@kavilla
[Build] fail silently on register git hook failure
41ac938 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
opensearch-project#851

Issue resolved:
opensearch-project#850

Signed-off-by: Kawika Avilla <[email protected]>
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 7, 2021
@kavilla
[Build] fail silently on register git hook failure
42211dd 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
opensearch-project#851

Issue resolved:
opensearch-project#850

Signed-off-by: Kawika Avilla <[email protected]>
@mihirsoni
Copy link
Contributor

It is hard to enforce unless it is enforced way to check, if we don't enforce it, we'll have lot PRs not having proper linting. Could you describe the errors and behaviors? Can we modify the script such it can support both version or we just mentioned what version of git is required.

kavilla added a commit that referenced this issue Oct 11, 2021
@kavilla
[Build] fail silently on register git hook failure
38ace12 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
#851

Issue resolved:
#850

Signed-off-by: Kawika Avilla <[email protected]>
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 11, 2021
@kavilla
[1.x][Build] fail silently on register git hook failure
a85bb5b 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
opensearch-project#851

Issue resolved:
opensearch-project#850

Backport PR:
opensearch-project#852

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla mentioned this issue Oct 11, 2021
Merged
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 11, 2021
@kavilla
[1.1][Build] fail silently on register git hook failure
533f86b 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
opensearch-project#851

Issue resolved:
opensearch-project#850

Backport PR:
opensearch-project#852

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla mentioned this issue Oct 11, 2021
Merged
kavilla added a commit that referenced this issue Oct 13, 2021
@kavilla
[1.x][Build] fail silently on register git hook failure
99d4c28 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
#851

Issue resolved:
#850

Backport PR:
#852

Signed-off-by: Kawika Avilla <[email protected]>
kavilla added a commit that referenced this issue Oct 13, 2021
@kavilla
[1.1][Build] fail silently on register git hook failure
9a095a8 
`node scripts/register_git_hook` fails depending on the installed
git version because it ends up calling `git rev-parse --git-common-dir`.
Depending on the git version, `--git-common-dir` returns the literal
string `--git-common-dir` when it is suppose to return $GIT_COMMON_DIR
if set and $GIT_DIR if the previous is not set. Then it proceeds to try
to install from a path that doesn't exist because it will use
`--git-common-dir` in the path.

The goal for this fix is to bypass hard failures in 1.x since I do
not believe it is appropriate to fail building something due to git.
But I do not want to just remove adding the pre-commit git hook
if people are already used to it being installed while bootstrapping.

Here, I propose the removal of git hooks on bootstrap in a 2+ version
of Dashboards:
#851

Issue resolved:
#850

Backport PR:
#852

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla
Copy link
Member Author

kavilla commented Oct 15, 2021 •
edited
Loading

Could you describe the errors and behaviors? Can we modify the script such it can support both version or we just mentioned what version of git is required.

Yeah so I have a work around merged that checks if the literal script for -git-common-dir returns '-git-common-dir' instead of a real value. it seems to be hit or miss on which versions, and sometimes there are regressions for git-common-dir. But that fails to return a value it tries to look for a path like pathtofile/--git-common-dir instead of for example pathtofile/.git

More details here: #850

But it feels a little too hacky. Could we split it out instead of automatically attaching it the bootstrap script and have it in the developer guide. Otherwise potentially we can add a new script and call it while building for a release build.

@mihirsoni
Copy link
Contributor

But it feels a little too hacky. Could we split it out instead of automatically attaching it the bootstrap script and have it in the developer guide.

Why can't we just enforce the newest git version for developers ? If it is not automated it will be hard for us to maintain and enforce everyone to follow the way. So let's dive deep on see how can we automate, IMO if git version is issue, I am fine putting that as requirement.

Otherwise potentially we can add a new script and call it while building for a release build.

IMO, this is too late stage to identify such errors.

@tmarkley tmarkley added the technical debt If not paid, jeapardizes long-term success and maintainability of the repository. label May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request technical debt If not paid, jeapardizes long-term success and maintainability of the repository.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mihirsoni @tmarkley @kavilla