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

Add pre-commit hooks script #43656

Merged
merged 11 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Thank you for your interest in contributing to Azure SDK for Java.

- To make code changes, or contribute something new, please follow the [GitHub Forks / Pull requests model](https://help.github.com/articles/fork-a-repo/): Fork the repo, make the change and propose it back by submitting a pull request.

- After cloning the repo, copy the [pre-commit hooks file](https://github.com/Azure/azure-sdk-for-java/tree/main/eng/scripts/pre-commit) (located at `eng/scripts/pre-commit`) to your local `.git/hooks/` directory. This will run some validations before your changes are committed.

- Refer to the [wiki](https://github.com/Azure/azure-sdk-for-java/wiki/Building#testing-for-spotbugs-and-checkstyle-issues) to learn about how Azure SDK for java generates CheckStyle, SpotBugs, Jacoco, and JavaDoc reports.

- There are two Maven projects in the repo. Refer to the [wiki](https://github.com/Azure/azure-sdk-for-java/wiki/Building#pomclientxml-vs-pomdataxml) to learn about project structure for each.
Expand Down
36 changes: 36 additions & 0 deletions eng/scripts/pre-commit
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

@srnagar can you give more context on this? In general we have avoided git hooks because they are very fragile and cannot be relied on. From all our other trials they have caused more problems than they have helped.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a MQ task. More details on this can be found in this issue - #41715

The main reason was to ensure we did some basic validation like cred scanner and spell checks done before the changes are committed as these are more sensitive and don't want it to be part of commit history. This PR only contains a script and doesn't automatically enable pre-commit hooks for everyone. It is an opt-in feature.

Could you please let me know what were some of the problems this caused in the past?

Copy link
Member

Choose a reason for hiding this comment

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

There are a number of issues:

  1. There is no way to automatically setup a git hook so they cannot be relied on because they may not be enabled.
  2. Running random tools during this pre-commit time cause random failures. Anytime a failure occurs in one of these tools the commits also fail. This leads to often needing to explicitly disabling the hooks do to get a commit in.

We have tried to use these in .NET and JS repos in the past and they both decided to revert using them because of a lot of issues. I also have had nothing but issues with these in my past teams as well which is why I don't recommend them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to close the loop on this, we discussed this in the Java team meeting and will keep the script (with some improvements) in the eng/scripts directory. This will not automatically be used and will allow users of the repo to opt in to using this, if needed. If we run into issues, it's easy to opt out.


# Fail on first error
set -e

# Get the list of changed files
changed_files=$(git diff --cached --name-only)

# Cred Scanner

# Setting up Cred Scanner Pre-Commit Git Hook locally requires installing Guardian and running the
# initialization first before the pre-commit hook can actually be used.
# https://eng.ms/docs/products/credential-risk-exposure-defense/solutions/credentials_in_code/precommit-git-hook

if [[ "${CredScanBinDirPosix}" ]]; then
echo "########### Running credential scanner ###########"
"${CredScanBinDirPosix}/CredScanGitHook" -r "${PWD}"
else
echo "To enable credential scanner pre-commit hook, follow the instructions here - https://eng.ms/docs/products/credential-risk-exposure-defense/solutions/credentials_in_code/precommit-git-hook"
fi

# Spell Check
echo "########### Running spell check ###########"
exec echo $changed_files | npx cspell --no-summary --no-progress --no-must-find-files --file-list stdin
echo "Spell check completed successfully"

# Validate library versions
echo "########### Validating package versions ###########"
pwsh eng/versioning/pom_file_version_scanner.ps1
echo "Package versions validation completed successfully"

# Verify links
echo "########### Validating links ###########"
for file in $changed_files; do
pwsh eng/common/scripts/Verify-Links.ps1 $file
done
Loading