[Merge/release] Add commit message linting feature

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.

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:
 

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")
+}

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
+}