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 make targets for common checks + fixes #661

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Add make targets for common checks + fixes #661

merged 1 commit into from
Feb 26, 2024

Conversation

radeksimko
Copy link
Member

This is to make it easier for existing and new maintainers and contributors to run checks on the codebase.

These targets can also run from CI which can be implemented as part of a separate PR.

Examples

make check
go run github.com/hashicorp/copywrite@latest headers --plan
Executing in dry-run mode. Rerun without the `--plan` flag to apply changes.

Using license identifier: MPL-2.0
Using copyright holder: HashiCorp, Inc.

Exempting the following search patterns:
hclsyntax/fuzz/testdata/**
hclwrite/fuzz/testdata/**
json/fuzz/testdata/**
specsuite/tests/**

The following files are missing headers:
go vet ./...
"/Users/radeksimko/gopath/src/github.com/hashicorp/hcl/scripts/gofmtcheck.sh"
make fix
go run github.com/hashicorp/copywrite@latest headers
Using license identifier: MPL-2.0
Using copyright holder: HashiCorp, Inc.

Exempting the following search patterns:
hclsyntax/fuzz/testdata/**
hclwrite/fuzz/testdata/**
json/fuzz/testdata/**
specsuite/tests/**

The following files are missing headers:
2024-02-16T15:25:27.173Z [INFO]  cli: ext/dynblock/options.go modified
gofmt -w ./

Notes

Please note that the license checks specifically are currently failing. That is being fixed in #659

This is to make it easier for existing and new maintainers and contributors to run checks on the codebase.
@radeksimko radeksimko requested a review from a team February 16, 2024 15:30
@@ -0,0 +1,18 @@
fmtcheck:
Copy link
Member

@radditude radditude Feb 24, 2024

Choose a reason for hiding this comment

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

I can't help but notice that this file is itself missing a copyright header 😆 I don't know if that matters - the copywrite tool doesn't seem unhappy about it so maybe there's a global allowlist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I don't think it matters at this point.

The reason is that the tool basically does not know how to add the header to makefiles, it's not that it would be intentionally ignored.

See https://github.com/hashicorp/copywrite/blob/0f077594eee1ea09d721a7e6fc9f07302aaff7fe/addlicense/main.go#L360-L392

Also the same issue applies to the upstream project our copywrite has forked, although it does have some pending PRs:

To be fair there is support for *.cmake but I don't have enough context/knowledge to tell what that is and what's the relationship with Makefile or GNUMakefile and I am not quite convinced there's a value in learning that just to add the license headers. I would perceive the licensing risk with makefiles in general to be relatively low in comparison with the rest of the codebase.

Feel free to correct me if I'm wrong in any of the above though!

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, it seems pretty harmless to leave the Makefile un-headered!

@radeksimko radeksimko merged commit c152f81 into main Feb 26, 2024
7 checks passed
@radeksimko radeksimko deleted the add-makefile branch February 26, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants