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 hook for running addlicense #1095

Closed
wants to merge 3 commits into from

Conversation

rickeylev
Copy link
Collaborator

Integrate the addlicense.sh script with the pre-commit config so that it automatically runs when git committing.

@alexeagle
Copy link
Contributor

The standard way to do this is expose a pre-commit hook from the addlicense repo, so that everyone can add it, and you don't need a shell script or a new .pre-commit-hooks.yaml here

@rickeylev
Copy link
Collaborator Author

Thanks for the pointer. I sent google/addlicense#143. Once accepted (I don't see why it wouldn't be...), we'll have to wait for them to do a release with a stable tag to use in the configs. I have another PR ready for when that happens.

In the meantime, I found the docs that tell how to correctly setup a local hook and updated the code to do that. PTAL.

@f0rmiga
Copy link
Collaborator

f0rmiga commented Feb 25, 2023

It bothers me that pre-commit doesn't use Bazel for the binaries. The best way to deal with this is:

  • Pull the binaries using http_archives.
  • Create an alias with selects to resolve the platform.
  • Create a shell wrapper that cd into BUILD_WORKSPACE_DIRECTORY and execute the binary:
#!/usr/bin/env bash

set -o errexit -o nounset

cd "${BUILD_WORKSPACE_DIRECTORY}"

exec "${ADDLICENSE_BINARY}" "$@"
  • Add the entry attribute to the pre-commit hook with entry: "bazel run //<pre-commit wrappers package>:addlicense --".

@alexeagle
Copy link
Contributor

Pre-commit running Bazel is not great due to pre-commit/pre-commit#1003 (comment)
I think it does a pretty good job of providing the hermetic tooling and there's no need for Bazel where there's no dependency graph IMO

@chrislovecnm
Copy link
Collaborator

Can we get this into a test? CI should fail on this.

@chrislovecnm
Copy link
Collaborator

@rickeylev ping

Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Dec 10, 2023
Copy link

This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

@github-actions github-actions bot closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? Will close in 30 days if there is no new activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants