diff --git a/README.md b/README.md index 40b7327..ebeb4b3 100644 --- a/README.md +++ b/README.md @@ -34,3 +34,109 @@ make build # Build the binary ``` If you are having trouble building this project please reference its .drone.yml file. Everything you need to know about building LGTM is defined in that file. + +### Usage + +#### .lgtm file + +Each repository managed by LGTM can have a .lgtm file in the root of the repository. This file provides the configuration properties that LGTM uses for the repository. If the .lgtm file is missing then default values are used for all properties. + +#### MAINTAINERS file + +Each repository managed by LGTM should have a MAINTAINERS file that specifies who is allowed to approve pull requests. + +For simple approval, you can use a flat-file format for the list of approvers: +``` +Brad Rydzewski (@bradrydzewski) +Matt Norris (@mattnorris) +``` + +The name and email are optional, but recommended. The login is required. + +If only the login is specified, do not put the login in parenthesis or prefix it with an @. + +To use organizational approval, you must use the TOML file format to specify the members of each team. This format looks like: +``` +[people] + [people.bob] + name = "Bob Bobson" + email = "bob@lgtm.co" + login = "bob" + [people.fred] + name = "Fred Fredson" + email = "fred@lgtm.co" + login = "fred" + [people.jon] + name = "Jon Jonson" + email = "jon@lgtm.co" + login = "jon" + [people.ralph] + name = "Ralph Ralphington" + email = "ralph@lgtm.co" + login = "ralph" + [people.george] + name = "George McGeorge" + email = "george@lgtm.co" + login = "george" + +[org] + [org.cap] + people = [ + "bob", + "fred", + "jon" + ] + + [org.iron] + people = [ + "ralph", + "george" + ] +``` + +If no MAINTAINERS file is present, LGTM will fall back to using a github team with the name MAINTAINERS. You cannot enable organizational approval without a MAINTAINERS file. + +#### Approval Management + +In order for LGTM to pass its status check there must be a number of valid approvals. This is controlled by the `approvals` property in the .lgtm file: +``` +approvals = 2 +``` + +If this property is not specified, it defaults to the value 2. + +The message that approvers use to signal that they have granted approval is controlled by the `pattern` property in the .lgtm file: +``` +pattern = '(?i)LGTM' +``` + +If no pattern is specified, `(?i)LGTM` is used as the pattern. + +##### Simple approval + +By default, any approver on the list +``` +approval_algorithm = "simple" +``` + +If you want to prevent the creator of a pull request from approving their own pull request, set the `self_approval_off` property in the .lgtm file: + +``` +self_approval_off = true +``` + +This property defaults to false; i.e., if you are on the approver list, you can approve a pull request that you create. + +##### Organizational approval + +Organizational approval means that at most one person from a specified organizational team can approve a pull request. + +If self-approval is disabled (`self_approval_off = true`), then no one on the same team as the author can approve the pull request. + +It's possible to configure organizations so that a person is on multiple teams, but it is not recommended, as the team assignment recognized by LGTM is undefined. + +To enable organizational approval, add the following line to the .lgtm file: + +``` +approval_algorithm = "org" +``` diff --git a/approval/approval.go b/approval/approval.go new file mode 100644 index 0000000..7da2de5 --- /dev/null +++ b/approval/approval.go @@ -0,0 +1,73 @@ +package approval + +import ( + "fmt" + log "github.com/Sirupsen/logrus" + "github.com/lgtmco/lgtm/model" + "regexp" + "strings" +) + +// Func takes in the information needed to figure out which approvers were in the PR comments +// and returns a slice of the approvers that were found +type Func func(*model.Config, *model.Maintainer, *model.Issue, []*model.Comment, Processor) + +var approvalMap = map[string]Func{} + +func Register(name string, f Func) error { + if _, ok := approvalMap[strings.ToLower(name)]; ok { + return fmt.Errorf("Approval Algorithm %s is already registered.", name) + } + approvalMap[strings.ToLower(name)] = f + log.Debug("added to approvalMap:", name, f) + return nil +} + +func Lookup(name string) (Func, error) { + log.Debug("approvalMap has", approvalMap) + log.Debugf("looking for '%s'\n", name) + f, ok := approvalMap[strings.ToLower(name)] + if !ok { + return nil, fmt.Errorf("Unknown Approval Algorithm %s", name) + } + return f, nil +} + +func init() { + Register("simple", Simple) +} + +type Processor func(*model.Maintainer, *model.Comment) + +// Simple is a helper function that analyzes the list of comments +// and finds the ones that have approvers on the maintainers list. +func Simple(config *model.Config, maintainer *model.Maintainer, issue *model.Issue, comments []*model.Comment, p Processor) { + approverm := map[string]bool{} + + matcher, err := regexp.Compile(config.Pattern) + if err != nil { + // this should never happen + return + } + + for _, comment := range comments { + // cannot lgtm your own pull request + if config.SelfApprovalOff && comment.Author == issue.Author { + continue + } + // the user must be a valid maintainer of the project + _, ok := maintainer.People[comment.Author] + if !ok { + continue + } + // the same author can't approve something twice + if _, ok := approverm[comment.Author]; ok { + continue + } + // verify the comment matches the approval pattern + if matcher.MatchString(comment.Body) { + approverm[comment.Author] = true + p(maintainer, comment) + } + } +} diff --git a/approval/org/org.go b/approval/org/org.go new file mode 100644 index 0000000..eb4eb07 --- /dev/null +++ b/approval/org/org.go @@ -0,0 +1,78 @@ +package org + +import ( + "github.com/lgtmco/lgtm/approval" + "github.com/lgtmco/lgtm/model" + "regexp" +) + +func init() { + approval.Register("org", Org) +} + +// Org is a helper function that analyzes the list of comments +// and returns the list of approvers. +// The rules for Org are: +// - At most one approver from each team +// - If SelfApprovalOff is true, no member of the team of the creator of the Pull Request is allowed to approve the PR +// - If a person appears on more than one team, they only count once, for the first team in which they appear +// (not solving the optimal grouping problem yet) +func Org(config *model.Config, maintainer *model.Maintainer, issue *model.Issue, comments []*model.Comment, p approval.Processor) { + //groups that have already approved + approvergm := map[string]bool{} + + //org that the author belongs to + authorOrg := "" + + //key is person, value is map of the orgs for that person + orgMap := map[string]map[string]bool{} + + //key is org name, value is org + for k, v := range maintainer.Org { + //value is name of person in the org + for _, name := range v.People { + if name == issue.Author { + authorOrg = k + } + m, ok := orgMap[name] + if !ok { + m = map[string]bool{} + orgMap[name] = m + } + m[k] = true + } + } + + matcher, err := regexp.Compile(config.Pattern) + if err != nil { + // this should never happen + return + } + + for _, comment := range comments { + // verify the comment matches the approval pattern + if !matcher.MatchString(comment.Body) { + continue + } + //get the orgs for the current comment's author + curOrgs, ok := orgMap[comment.Author] + if !ok { + // the user must be a valid maintainer of the project + continue + } + for curOrg := range curOrgs { + // your group cannot lgtm your own pull request + if config.SelfApprovalOff && curOrg == authorOrg { + continue + } + // your group cannot approve twice + if approvergm[curOrg] { + continue + } + //found one! + approvergm[curOrg] = true + p(maintainer, comment) + } + } + return +} diff --git a/approval/org/org_test.go b/approval/org/org_test.go new file mode 100644 index 0000000..ecb59e5 --- /dev/null +++ b/approval/org/org_test.go @@ -0,0 +1,80 @@ +package org + +import ( + "github.com/lgtmco/lgtm/model" + "testing" +) + +var maintainerToml = ` +[people] + [people.bob] + login = "bob" + [people.fred] + login = "fred" + [people.jon] + login = "jon" + [people.ralph] + login = "ralph" + [people.george] + login = "george" + +[org] +[org.cap] +people = [ + "bob", + "fred", + "jon" +] + +[org.iron] +people = [ + "ralph", + "george" +] +` + +func TestOrg(t *testing.T) { + config := &model.Config{ + Pattern: `(?i)LGTM\s*(\S*)`, + SelfApprovalOff: true, + } + m, err := model.ParseMaintainerStr(maintainerToml) + if err != nil { + t.Fatal(err) + } + issue := &model.Issue{ + Author: "jon", + } + comments := []*model.Comment{ + { + Body: "lgtm", + Author: "bob", + }, + { + Body: "lgtm", + Author: "qwerty", + }, + { + Body: "not an approval", + Author: "ralph", + }, + { + Body: "lgtm", + Author: "george", + }, + { + Body: "lgtm", + Author: "ralph", + }, + } + people := []string{} + Org(config, m, issue, comments, func(m *model.Maintainer, c *model.Comment) { + people = append(people, c.Author) + }) + if len(people) != 1 { + t.Errorf("Expected one person, had %d", len(people)) + } + if people[0] != "george" { + t.Errorf("Expected george, had %s", people[0]) + } +} diff --git a/main.go b/main.go index e63e57c..9f47810 100644 --- a/main.go +++ b/main.go @@ -4,6 +4,7 @@ import ( "net/http" "time" + _ "github.com/lgtmco/lgtm/approval/org" "github.com/lgtmco/lgtm/router" "github.com/lgtmco/lgtm/router/middleware" diff --git a/model/config.go b/model/config.go index bf6d63e..2f26c35 100644 --- a/model/config.go +++ b/model/config.go @@ -11,6 +11,7 @@ type Config struct { Approvals int `json:"approvals" toml:"approvals"` Pattern string `json:"pattern" toml:"pattern"` Team string `json:"team" toml:"team"` + ApprovalAlg string `json:"approval_algorithm" toml:"approval_algorithm"` SelfApprovalOff bool `json:"self_approval_off" toml:"self_approval_off"` re *regexp.Regexp @@ -20,6 +21,7 @@ var ( approvals = envflag.Int("LGTM_APPROVALS", 2, "") pattern = envflag.String("LGTM_PATTERN", "(?i)LGTM", "") team = envflag.String("LGTM_TEAM", "MAINTAINERS", "") + approvalAlg = envflag.String("LGTM_APPROVAL_ALGORITHM", "simple", "") selfApprovalOff = envflag.Bool("LGTM_SELF_APPROVAL_OFF", false, "") ) @@ -44,6 +46,9 @@ func ParseConfigStr(data string) (*Config, error) { if len(c.Team) == 0 { c.Team = *team } + if len(c.ApprovalAlg) == 0 { + c.ApprovalAlg = *approvalAlg + } if c.SelfApprovalOff == false { c.SelfApprovalOff = *selfApprovalOff } diff --git a/web/comment_hook.go b/web/comment_hook.go new file mode 100644 index 0000000..91a0afe --- /dev/null +++ b/web/comment_hook.go @@ -0,0 +1,71 @@ +package web + +import ( + log "github.com/Sirupsen/logrus" + "github.com/gin-gonic/gin" + "github.com/lgtmco/lgtm/approval" + "github.com/lgtmco/lgtm/model" + "github.com/lgtmco/lgtm/remote" +) + +func processCommentHook(c *gin.Context, hook *model.Hook) { + repo, user, err := getRepoAndUser(c, hook.Repo.Slug) + if err != nil { + return + } + + config, maintainer, err := getConfigAndMaintainers(c, user, repo) + if err != nil { + return + } + + approvers, err := buildApprovers(c, user, repo, config, maintainer, hook.Issue) + if err != nil { + return + } + + approved := len(approvers) >= config.Approvals + err = remote.SetStatus(c, user, repo, hook.Issue.Number, approved) + if err != nil { + log.Errorf("Error setting status for %s pr %d. %s", repo.Slug, hook.Issue.Number, err) + c.String(500, "Error setting status. %s.", err) + return + } + + log.Debugf("processed comment for %s. received %d of %d approvals", repo.Slug, len(approvers), config.Approvals) + + c.IndentedJSON(200, gin.H{ + "approvers": maintainer.People, + "settings": config, + "approved": approved, + "approved_by": approvers, + }) +} + +func buildApprovers(c *gin.Context, user *model.User, repo *model.Repo, config *model.Config, maintainer *model.Maintainer, issue *model.Issue) ([]*model.Person, error) { + comments, err := getComments(c, user, repo, issue.Number) + if err != nil { + log.Errorf("Error getting comments for %s/%s/%d", repo.Owner, repo.Name, issue.Number) + c.String(500, "Error getting comments for %s/%s/%d", repo.Owner, repo.Name, issue.Number) + return nil, err + } + + alg, err := approval.Lookup(config.ApprovalAlg) + if err != nil { + log.Errorf("Error getting approval algorithm %s. %s", config.ApprovalAlg, err) + c.String(500, "Error getting approval algorithm %s. %s", config.ApprovalAlg, err) + return nil, err + } + approvers := getApprovers(config, maintainer, issue, comments, alg) + return approvers, nil +} + +// getApprovers is a helper function that analyzes the list of comments +// and returns the list of approvers. +func getApprovers(config *model.Config, maintainer *model.Maintainer, issue *model.Issue, comments []*model.Comment, matcher approval.Func) []*model.Person { + approvers := []*model.Person{} + matcher(config, maintainer, issue, comments, func(maintainer *model.Maintainer, comment *model.Comment) { + approvers = append(approvers, maintainer.People[comment.Author]) + }) + return approvers +} diff --git a/web/hook.go b/web/hook.go index 1378415..71e26c8 100644 --- a/web/hook.go +++ b/web/hook.go @@ -1,8 +1,6 @@ package web import ( - "regexp" - "github.com/lgtmco/lgtm/cache" "github.com/lgtmco/lgtm/model" "github.com/lgtmco/lgtm/remote" @@ -13,36 +11,45 @@ import ( ) func Hook(c *gin.Context) { - hook, err := remote.GetHook(c, c.Request) + commentHook, err := remote.GetHook(c, c.Request) if err != nil { log.Errorf("Error parsing hook. %s", err) c.String(500, "Error parsing hook. %s", err) return } - if hook == nil { + if commentHook != nil { + processCommentHook(c, commentHook) + } + // Impl note: test all hooks for nil + if commentHook == nil { c.String(200, "pong") return } +} - repo, err := store.GetRepoSlug(c, hook.Repo.Slug) +func getRepoAndUser(c *gin.Context, slug string) (*model.Repo, *model.User, error) { + repo, err := store.GetRepoSlug(c, slug) if err != nil { - log.Errorf("Error getting repository %s. %s", hook.Repo.Slug, err) + log.Errorf("Error getting repository %s. %s", slug, err) c.String(404, "Repository not found.") - return + return nil, nil, err } user, err := store.GetUser(c, repo.UserID) if err != nil { log.Errorf("Error getting repository owner %s. %s", repo.Slug, err) c.String(404, "Repository owner not found.") - return + return nil, nil, err } + return repo, user, err +} +func getConfigAndMaintainers(c *gin.Context, user *model.User, repo *model.Repo) (*model.Config, *model.Maintainer, error) { rcfile, _ := remote.GetContents(c, user, repo, ".lgtm") config, err := model.ParseConfig(rcfile) if err != nil { log.Errorf("Error parsing .lgtm file for %s. %s", repo.Slug, err) c.String(500, "Error parsing .lgtm file. %s.", err) - return + return nil, nil, err } // THIS IS COMPLETELY DUPLICATED IN THE API SECTION. NOT IDEAL @@ -54,7 +61,7 @@ func Hook(c *gin.Context) { log.Errorf("Error getting repository %s. %s", repo.Slug, err) log.Errorf("Error getting org members %s. %s", repo.Owner, merr) c.String(404, "MAINTAINERS file not found. %s", err) - return + return nil, nil, err } else { for _, member := range members { file = append(file, member.Login...) @@ -67,66 +74,17 @@ func Hook(c *gin.Context) { if err != nil { log.Errorf("Error parsing MAINTAINERS file for %s. %s", repo.Slug, err) c.String(500, "Error parsing MAINTAINERS file. %s.", err) - return + return nil, nil, err } - - comments, err := remote.GetComments(c, user, repo, hook.Issue.Number) - if err != nil { - log.Errorf("Error retrieving comments for %s pr %d. %s", repo.Slug, hook.Issue.Number, err) - c.String(500, "Error retrieving comments. %s.", err) - return - } - approvers := getApprovers(config, maintainer, hook.Issue, comments) - approved := len(approvers) >= config.Approvals - err = remote.SetStatus(c, user, repo, hook.Issue.Number, approved) - if err != nil { - log.Errorf("Error setting status for %s pr %d. %s", repo.Slug, hook.Issue.Number, err) - c.String(500, "Error setting status. %s.", err) - return - } - - log.Debugf("processed comment for %s. received %d of %d approvals", repo.Slug, len(approvers), config.Approvals) - - c.IndentedJSON(200, gin.H{ - "approvers": maintainer.People, - "settings": config, - "approved": approved, - "approved_by": approvers, - }) + return config, maintainer, nil } -// getApprovers is a helper function that analyzes the list of comments -// and returns the list of approvers. -func getApprovers(config *model.Config, maintainer *model.Maintainer, issue *model.Issue, comments []*model.Comment) []*model.Person { - approverm := map[string]bool{} - approvers := []*model.Person{} - - matcher, err := regexp.Compile(config.Pattern) +func getComments(c *gin.Context, user *model.User, repo *model.Repo, num int) ([]*model.Comment, error) { + comments, err := remote.GetComments(c, user, repo, num) if err != nil { - // this should never happen - return approvers - } - - for _, comment := range comments { - // cannot lgtm your own pull request - if config.SelfApprovalOff && comment.Author == issue.Author { - continue - } - // the user must be a valid maintainer of the project - person, ok := maintainer.People[comment.Author] - if !ok { - continue - } - // the same author can't approve something twice - if _, ok := approverm[comment.Author]; ok { - continue - } - // verify the comment matches the approval pattern - if matcher.MatchString(comment.Body) { - approverm[comment.Author] = true - approvers = append(approvers, person) - } + log.Errorf("Error retrieving comments for %s pr %d. %s", repo.Slug, num, err) + c.String(500, "Error retrieving comments. %s.", err) + return nil, err } - - return approvers + return comments, nil }