diff --git a/GET.md b/GET.md index 18a1f74..3a3bc87 100644 --- a/GET.md +++ b/GET.md @@ -49,8 +49,23 @@ maintainers: features: - dco_check - comments + - commit_linting ``` +Features: + +* `dco_check` - checks that each commit finishes with a "Signed-off-by:" statement +* `comments` - allows `maintainers` to issue commands to Derek to add labels etc +* `commit_linting` - applies linting rules to commit messages + +Commit linting rules: + +- Commit subject should not exceed 72 characters +- Commit subject should start with an uppercase letter +- Commit subject should not end with punctuation + +The linting rules are based upon a [blog post by Chris Beams](https://chris.beams.io/posts/git-commit/) + **Testing** Create a label of "no-dco" within every project you want Derek to help you with. diff --git a/README.md b/README.md index eaa18f3..5866dea 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,7 @@ Derek unlock * [x] Lock thread * [x] Edit title * [x] Toggle the DCO-feature +* [x] Add commit message linting feature Future work: diff --git a/main.go b/main.go index fe23550..6aa1470 100644 --- a/main.go +++ b/main.go @@ -11,15 +11,8 @@ import ( "github.com/alexellis/derek/types" ) -const dcoCheck = "dco_check" -const comments = "comments" const deleted = "deleted" -func hmacValidation() bool { - val := os.Getenv("validate_hmac") - return len(val) > 0 && (val == "1" || val == "true") -} - func main() { bytesIn, _ := ioutil.ReadAll(os.Stdin) @@ -53,7 +46,7 @@ func handleEvent(eventType string, bytesIn []byte) error { case "pull_request": req := types.PullRequestOuter{} if err := json.Unmarshal(bytesIn, &req); err != nil { - return fmt.Errorf("Cannot parse input %s", err.Error()) + return fmt.Errorf("pull_request handler, cannot parse input: %s", err.Error()) } customer, err := auth.IsCustomer(req.Repository) @@ -67,11 +60,18 @@ func handleEvent(eventType string, bytesIn []byte) error { if err != nil { return fmt.Errorf("Unable to access maintainers file at: %s/%s", req.Repository.Owner.Login, req.Repository.Name) } + if req.Action != closedConstant { - if enabledFeature(dcoCheck, derekConfig) { - handlePullRequest(req) + prFeatures := types.PullRequestFeatures{ + CommitLintingFeature: enabledFeature(types.CommitLintingFeature, derekConfig), + DCOCheckFeature: enabledFeature(types.DCOCheckFeature, derekConfig), + } + + if prFeatures.Enabled() { + handlePullRequest(req, prFeatures) } } + break case "issue_comment": @@ -93,14 +93,21 @@ func handleEvent(eventType string, bytesIn []byte) error { } if req.Action != deleted { - if permittedUserFeature(comments, derekConfig, req.Comment.User.Login) { + if permittedUserFeature(types.CommentFeature, derekConfig, req.Comment.User.Login) { handleComment(req) } } + break + default: return fmt.Errorf("X_Github_Event want: ['pull_request', 'issue_comment'], got: " + eventType) } return nil } + +func hmacValidation() bool { + val := os.Getenv("validate_hmac") + return len(val) > 0 && (val == "1" || val == "true") +} diff --git a/pullRequestHandler.go b/pullRequestHandler.go index d16bc5a..964bd46 100644 --- a/pullRequestHandler.go +++ b/pullRequestHandler.go @@ -13,7 +13,11 @@ import ( "github.com/google/go-github/github" ) -func handlePullRequest(req types.PullRequestOuter) { +// maximumCommitSubjectLength is set this way because Chris beams advises +// 50 but the GitHub UI will support 72 +const maximumCommitSubjectLength = 72 + +func handlePullRequest(req types.PullRequestOuter, prFeatures types.PullRequestFeatures) { ctx := context.Background() token := os.Getenv("access_token") @@ -32,88 +36,98 @@ func handlePullRequest(req types.PullRequestOuter) { client := auth.MakeClient(ctx, token) - hasUnsignedCommits, err := hasUnsigned(req, client) + commits, commitFetchErr := getCommits(req, client) - if err != nil { - log.Fatal(err) - } else if hasUnsignedCommits { - fmt.Println("May need to apply labels on item.") + if commitFetchErr != nil { + log.Fatal(commitFetchErr) + } - issue, _, labelErr := client.Issues.Get(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number) + issue, _, labelErr := client.Issues.Get(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number) + if labelErr != nil { + log.Fatalln("Unable to fetch labels for PR %s/%s/#%d", req.Repository.Owner, req.Repository.Name, req.PullRequest.Number) + } - if labelErr != nil { - log.Fatalln(labelErr) + if prFeatures.CommitLintingFeature { + lintResult := lintCommits(commits) + applyErr := applyLintingLabel(req, client, issue, lintResult) + if applyErr != nil { + log.Printf("Error applying linting rule: %s", applyErr) } - fmt.Println("Current labels ", issue.Labels) + } - if hasNoDcoLabel(issue) == false { - fmt.Println("Applying label") - _, res, assignLabelErr := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, []string{"no-dco"}) - if assignLabelErr != nil { - log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, res.Limit, res.Remaining) - } + if prFeatures.DCOCheckFeature { + if hasUnsigned(commits) == true { + fmt.Println("May need to apply labels on item.") - link := fmt.Sprintf("https://github.com/%s/%s/blob/master/CONTRIBUTING.md", req.Repository.Owner.Login, req.Repository.Name) - body := `Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. -That's something we need before your Pull Request can be merged. Please see our [contributing guide](` + link + `).` + fmt.Println("Current labels ", issue.Labels) - comment := &github.IssueComment{ - Body: &body, - } + if hasLabelAssigned("no-dco", issue) == false { + fmt.Println("Applying label") + _, res, assignLabelErr := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, []string{"no-dco"}) + if assignLabelErr != nil { + log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, res.Limit, res.Remaining) + } - comment, resp, err := client.Issues.CreateComment(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, comment) - if err != nil { - log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, resp.Limit, resp.Remaining) - log.Fatal(err) - } - fmt.Println(comment, resp.Rate) - } - } else { - fmt.Println("Things look OK right now.") - issue, res, labelErr := client.Issues.Get(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number) + link := fmt.Sprintf("https://github.com/%s/%s/blob/master/CONTRIBUTING.md", req.Repository.Owner.Login, req.Repository.Name) + body := `Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. +That's something we need before your Pull Request can be merged. Please see our [contributing guide](` + link + `).` - if labelErr != nil { - log.Fatalf("%s limit: %d, remaining: %d", labelErr, res.Limit, res.Remaining) - log.Fatalln() - } + comment := &github.IssueComment{ + Body: &body, + } - if hasNoDcoLabel(issue) { - fmt.Println("Removing label") - _, removeLabelErr := client.Issues.RemoveLabelForIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, "no-dco") - if removeLabelErr != nil { - log.Fatal(removeLabelErr) + comment, resp, err := client.Issues.CreateComment(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, comment) + if err != nil { + log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, resp.Limit, resp.Remaining) + log.Fatal(err) + } + fmt.Println(comment, resp.Rate) + } + } else { + fmt.Println("Things look OK right now.") + + if hasLabelAssigned("no-dco", issue) { + fmt.Println("Removing label") + _, removeLabelErr := client.Issues.RemoveLabelForIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, "no-dco") + if removeLabelErr != nil { + log.Fatal(removeLabelErr) + } } } } } -func hasNoDcoLabel(issue *github.Issue) bool { +func hasLabelAssigned(labelName string, issue *github.Issue) bool { if issue != nil { for _, label := range issue.Labels { - if label.GetName() == "no-dco" { + if label.GetName() == labelName { return true } } } + return false } -func hasUnsigned(req types.PullRequestOuter, client *github.Client) (bool, error) { - hasUnsigned := false +func getCommits(req types.PullRequestOuter, client *github.Client) ([]*github.RepositoryCommit, error) { ctx := context.Background() - var err error + var responseErr error listOpts := &github.ListOptions{ Page: 0, } commits, resp, err := client.PullRequests.ListCommits(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, listOpts) if err != nil { - log.Fatalf("Error getting PR %d\n%s", req.PullRequest.Number, err.Error()) - return hasUnsigned, err + responseErr = fmt.Errorf("unable to fetch commits for PR: %d, Error: %s", req.PullRequest.Number, err.Error()) } - fmt.Println("Rate limiting", resp.Rate) + log.Printf("Rate limiting remaining: %d", resp.Rate.Remaining) + return commits, responseErr +} + +func hasUnsigned(commits []*github.RepositoryCommit) bool { + hasUnsigned := false for _, commit := range commits { if commit.Commit != nil && commit.Commit.Message != nil { @@ -123,9 +137,108 @@ func hasUnsigned(req types.PullRequestOuter, client *github.Client) (bool, error } } - return hasUnsigned, err + return hasUnsigned } func isSigned(msg string) bool { return strings.Contains(msg, "Signed-off-by:") } + +func lintCommits(commits []*github.RepositoryCommit) bool { + for _, commit := range commits { + if commit.Commit != nil && commit.Commit.Message != nil { + if lintCommit(commit.Commit.Message) == false { + return false + } + } + } + return true +} + +func lintCommit(message *string) bool { + var valid bool + + if message == nil || len(*message) == 0 { + return false + } + + parts := strings.Split(*message, "\n") + + if len(parts) > 0 { + firstLine := parts[0] + lengthValid := len(firstLine) <= maximumCommitSubjectLength + + var startsWithUpper bool + var endsWithPunctuation bool + + firstCharacter := getFirstCharacter(firstLine) + if firstCharacter != nil { + startsWithUpper = len(*firstCharacter) > 0 && strings.ToUpper(*firstCharacter) == *firstCharacter + } + + if len(firstLine) > 0 { + endsWithPunctuation = strings.LastIndexAny(firstLine, ".!") == len(firstLine)-1 + } + + valid = lengthValid && startsWithUpper && !endsWithPunctuation + } + + return valid +} + +func getFirstCharacter(msg string) *string { + var ret *string + + for _, runeVal := range msg { + asStr := string(runeVal) + ret = &asStr + break + } + + return ret +} + +func applyLintingLabel(req types.PullRequestOuter, client *github.Client, issue *github.Issue, lintResult bool) error { + labelCaption := "review/commit-message" + var actionErr error + hasLabel := hasLabelAssigned(labelCaption, issue) + + ctx := context.Background() + + if hasLabel { + if lintResult == true { + res, assignLabelErr := client.Issues.RemoveLabelForIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, labelCaption) + if assignLabelErr != nil { + actionErr = fmt.Errorf("removeLabel: %s limit: %d, remaining: %d", assignLabelErr, res.Limit, res.Remaining) + } + } + } else { + if lintResult == false { + _, res, assignLabelErr := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, []string{labelCaption}) + if assignLabelErr != nil { + actionErr = fmt.Errorf("addLabel: %s limit: %d, remaining: %d", assignLabelErr, res.Limit, res.Remaining) + } + + link := fmt.Sprintf("https://github.com/%s/%s/blob/master/CONTRIBUTING.md#commit-messages", req.Repository.Owner.Login, req.Repository.Name) + + body := `Please check that your commit messages fit within [these guidelines](` + link + `): +- Commit subject should not exceed 72 characters +- Commit subject should start with an uppercase letter +- Commit subject should not end with punctuation + +Read [how to re-write commit history](https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History). +` + + comment := &github.IssueComment{ + Body: &body, + } + + _, _, err := client.Issues.CreateComment(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, comment) + if err != nil { + actionErr = fmt.Errorf("unable to create comment due to linting check: %s", err) + } + } + } + + return actionErr +} diff --git a/pullRequestHandler_test.go b/pullRequestHandler_test.go index 60c0b4b..3f82428 100644 --- a/pullRequestHandler_test.go +++ b/pullRequestHandler_test.go @@ -6,6 +6,63 @@ import ( "github.com/google/go-github/github" ) +func Test_LintingResultGiven_WithLongSubject(t *testing.T) { + + var testCases = []struct { + scenario string + message string + lintResult bool + }{ + { + scenario: "Commit subject over 72 chars", + message: "This commit is necessary to make sure that all future commits conform to a certain set pattern\nSigned-off-by: Alex Ellis ", + lintResult: false, + }, + { + scenario: "Commit subject exactly 72 chars", + message: "This commit subject falls well within the boundary with exactly the cor\nSigned-off-by: Alex Ellis ", + lintResult: true, + }, + { + scenario: "Commit subject ends in punctuation - (.)", + message: "Fixes #7071.\nSigned-off-by: Alex Ellis ", + lintResult: false, + }, + { + scenario: "Commit subject ends in punctuation - (!)", + message: "Fixes #7071!\nSigned-off-by: Alex Ellis ", + lintResult: false, + }, + { + scenario: "Commit subject starts with lowercase", + message: "has lowercase subject\nSigned-off-by: Alex Ellis ", + lintResult: false, + }, + { + scenario: "Commit subject within length boundaries and punctuation (.) mid-way", + message: "Fixes. #7071\nSigned-off-by: Alex Ellis ", + lintResult: true, + }, + { + scenario: "Empty message given", + message: "", + lintResult: false, + }, + } + + for _, testCase := range testCases { + + testMsg := testCase.message + result := lintCommit(&testMsg) + + if result != testCase.lintResult { + t.Logf("scenario: %s - want linting: %v, but got: %v\n message: %s", testCase.scenario, testCase.lintResult, result, testCase.message) + t.Fail() + } + } + +} + func Test_isSigned(t *testing.T) { var signOffOpts = []struct { @@ -34,6 +91,7 @@ func Test_isSigned(t *testing.T) { expectedBool: false, }, } + for _, test := range signOffOpts { t.Run(test.title, func(t *testing.T) { @@ -84,7 +142,7 @@ func Test_hasNoDcoLabel(t *testing.T) { inputIssue := &github.Issue{Labels: ghLabels} - hasLabel := hasNoDcoLabel(inputIssue) + hasLabel := hasLabelAssigned("no-dco", inputIssue) if hasLabel != test.expectedBool { t.Errorf("Has no-dco label - wanted: %t, found %t", test.expectedBool, hasLabel) diff --git a/types/features.go b/types/features.go new file mode 100644 index 0000000..d028725 --- /dev/null +++ b/types/features.go @@ -0,0 +1,15 @@ +package types + +const DCOCheckFeature = "dco_check" +const CommitLintingFeature = "commit_linting" +const CommentFeature = "comments" + +// PullRequestFeatures +type PullRequestFeatures struct { + DCOCheckFeature bool + CommitLintingFeature bool +} + +func (p *PullRequestFeatures) Enabled() bool { + return p.DCOCheckFeature || p.CommitLintingFeature +}