Repository: palantir/policy-bot (go)

Issue:
Add an optional description to rules
Some more complicated rules might require a few sentences to describe to users what sort of approvals are required to satisfy the rule. It is possible to use a complicated rule name to describe this, but it's not nice for two reasons
1. The name gets repeated in the `.policy.approval` list. When the long name gets updated because someone wants to reword the approval requirements, they need to remember to make the same change to this list.
2. The UI looks strange with a long name as the large, bold text suggests it was probably designed around the assumption that the name would be short.

### Proposal

Add an optional `description` field to rules which can be used to store the detailed information for users about what is required to get the approval rule to pass. Since this field is optional, the change would be backwards compatible and have no effect for existing configurations of policy-bot.

As an example, consider the following config file:

```yaml
policy:
  approval:
  - Team 1
  - Team 2
  - Admin

approval_rules:
- name: Team 1
  description: Changes to config must be approved by your own team
  requires:
    count: 1
    teams:
    - myorg/team1
  if:
    changed_files:
      paths:
      - '^teams/team1/.*'
- name: Team 2
  description: Changes to config must be approved by your own team
  requires:
    count: 1
    teams:
    - myorg/team2
  if:
    changed_files:
      paths:
      - '^teams/team2/.*'
- name: Admin
  description: All other changes must be approved by an admin
  requires:
    count: 1
    teams:
    - myorg/admin
  if:
    changed_files:
      paths:
      - '.*'
      ignore:
      - '^teams/.*'
```

Which could look something like this:

![image](https://user-images.githubusercontent.com/2044464/100578376-21dafd00-331d-11eb-8228-a962a816b877.png)



Pull request:
Add optional description field to approval rules
This change adds an optional description field to approval rules. This description is only displayed when the field is non-empty and the rule has not been skipped.

Closes #250

Patch:
diff --git a/config/policy-examples/complicated.yml b/config/policy-examples/complicated.yml
index 5ee7528..65e8d5d 100644
--- a/config/policy-examples/complicated.yml
+++ b/config/policy-examples/complicated.yml
@@ -4,17 +4,19 @@
 policy:
   approval:
   - or:
-    - catskills can approve
-    - devtools can approve changes to staging
+    - catskills
+    - devtools
 
 approval_rules:
 
-- name: catskills can approve
+- name: catskills
+  description: catskills can approve
   requires:
     count: 1
     teams: ["palantir/catskills"]
 
-- name: devtools can approve changes to staging
+- name: devtools
+  description: devtools can approve changes to staging
   if:
     only_changed_files:
       paths:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index db6a5a8..6c6eca0 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -29,10 +29,11 @@ import (
 )
 
 type Rule struct {
-	Name       string     `yaml:"name"`
-	Predicates Predicates `yaml:"if"`
-	Options    Options    `yaml:"options"`
-	Requires   Requires   `yaml:"requires"`
+	Name        string     `yaml:"name"`
+	Description string     `yaml:"description"`
+	Predicates  Predicates `yaml:"if"`
+	Options     Options    `yaml:"options"`
+	Requires    Requires   `yaml:"requires"`
 }
 
 type Options struct {
@@ -99,6 +100,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 	log := zerolog.Ctx(ctx)
 
 	res.Name = r.Name
+	res.Description = r.Description
 	res.Status = common.StatusSkipped
 
 	for _, p := range r.Predicates.Predicates() {
@@ -111,9 +113,9 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		if !satisfied {
 			log.Debug().Msgf("skipping rule, predicate of type %T was not satisfied", p)
 
-			res.Description = desc
+			res.StatusDescription = desc
 			if desc == "" {
-				res.Description = "The preconditions of this rule are not satisfied"
+				res.StatusDescription = "The preconditions of this rule are not satisfied"
 			}
 
 			return
@@ -126,7 +128,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		return
 	}
 
-	res.Description = msg
+	res.StatusDescription = msg
 	if approved {
 		res.Status = common.StatusApproved
 	} else {
diff --git a/policy/approval/evaluate.go b/policy/approval/evaluate.go
index abd02ba..9e0e144 100644
--- a/policy/approval/evaluate.go
+++ b/policy/approval/evaluate.go
@@ -42,7 +42,7 @@ func (eval *evaluator) Evaluate(ctx context.Context, prctx pull.Context) (res co
 		zerolog.Ctx(ctx).Debug().Msg("No approval policy defined; skipping")
 
 		res.Status = common.StatusApproved
-		res.Description = "No approval policy defined"
+		res.StatusDescription = "No approval policy defined"
 	}
 
 	res.Name = "approval"
@@ -63,7 +63,7 @@ func (r *RuleRequirement) Evaluate(ctx context.Context, prctx pull.Context) comm
 
 	result := r.rule.Evaluate(ctx, prctx)
 	if result.Error == nil {
-		log.Debug().Msgf("rule evaluation resulted in %s:\"%s\"", result.Status, result.Description)
+		log.Debug().Msgf("rule evaluation resulted in %s:\"%s\"", result.Status, result.StatusDescription)
 	}
 
 	return result
@@ -121,11 +121,11 @@ func (r *OrRequirement) Evaluate(ctx context.Context, prctx pull.Context) common
 	}
 
 	return common.Result{
-		Name:        "or",
-		Status:      status,
-		Description: description,
-		Error:       err,
-		Children:    children,
+		Name:              "or",
+		Status:            status,
+		StatusDescription: description,
+		Error:             err,
+		Children:          children,
 	}
 }
 
@@ -179,10 +179,10 @@ func (r *AndRequirement) Evaluate(ctx context.Context, prctx pull.Context) commo
 	}
 
 	return common.Result{
-		Name:        "and",
-		Status:      status,
-		Description: description,
-		Error:       err,
-		Children:    children,
+		Name:              "and",
+		Status:            status,
+		StatusDescription: description,
+		Error:             err,
+		Children:          children,
 	}
 }
diff --git a/policy/common/result.go b/policy/common/result.go
index 8accdbd..71c6bc0 100644
--- a/policy/common/result.go
+++ b/policy/common/result.go
@@ -57,10 +57,11 @@ type ReviewRequestRule struct {
 }
 
 type Result struct {
-	Name        string
-	Description string
-	Status      EvaluationStatus
-	Error       error
+	Name              string
+	Description       string
+	StatusDescription string
+	Status            EvaluationStatus
+	Error             error
 
 	ReviewRequestRule *ReviewRequestRule
 
diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go
index 54b41a8..64f1ed1 100644
--- a/policy/disapproval/disapprove.go
+++ b/policy/disapproval/disapprove.go
@@ -99,7 +99,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
 	if p.Requires.IsEmpty() {
 		log.Debug().Msg("no users are allowed to disapprove; skipping")
 
-		res.Description = "No disapproval policy is specified or the policy is empty"
+		res.StatusDescription = "No disapproval policy is specified or the policy is empty"
 		return
 	}
 
@@ -109,7 +109,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
 		return
 	}
 
-	res.Description = msg
+	res.StatusDescription = msg
 	if disapproved {
 		res.Status = common.StatusDisapproved
 	} else {
diff --git a/policy/policy.go b/policy/policy.go
index eedb757..a1e436e 100644
--- a/policy/policy.go
+++ b/policy/policy.go
@@ -93,10 +93,10 @@ func (e evaluator) Evaluate(ctx context.Context, prctx pull.Context) (res common
 	case res.Error != nil:
 	case disapproval.Status == common.StatusDisapproved:
 		res.Status = common.StatusDisapproved
-		res.Description = disapproval.Description
+		res.StatusDescription = disapproval.StatusDescription
 	default:
 		res.Status = approval.Status
-		res.Description = approval.Description
+		res.StatusDescription = approval.StatusDescription
 	}
 	return
 }
diff --git a/server/handler/base.go b/server/handler/base.go
index b5d7f73..9d297cf 100644
--- a/server/handler/base.go
+++ b/server/handler/base.go
@@ -191,7 +191,7 @@ func (b *Base) EvaluateFetchedConfig(ctx context.Context, prctx pull.Context, cl
 		return err
 	}
 
-	statusDescription := result.Description
+	statusDescription := result.StatusDescription
 	var statusState string
 	switch result.Status {
 	case common.StatusApproved:
diff --git a/server/templates/details.html.tmpl b/server/templates/details.html.tmpl
index 8056f3d..62e4a0c 100644
--- a/server/templates/details.html.tmpl
+++ b/server/templates/details.html.tmpl
@@ -31,7 +31,7 @@
     <div class="status-banner {{$s}} flex flex-wrap items-center justify-between">
       <div>
         <h2 class="mb-1 text-lg font-bold">Status: {{$s | titlecase}}</h2>
-        <p>{{or .Result.Error .Result.Description}}</p>
+        <p>{{or .Result.Error .Result.StatusDescription}}</p>
       </div>
       <div class="p-2 rounded-md bg-white text-dark-gray1 text-sm text-shadow-none">
         <div class="toggle">
@@ -68,5 +68,8 @@
     <b class="font-bold">{{.Name}}</b>
     <span class="flex-none status-badge {{$s}}">{{$s | titlecase}}</span>
   </p>
-  <p class="text-dark-gray3 text-sm">{{or .Error .Description}}</p>
+  {{if (and .Description (ne $s "skipped"))}}
+  <p class="mb-2 text-dark-gray3 text-sm">{{.Description}}</p>
+  {{end}}
+  <p class="text-dark-gray3 text-sm">{{or .Error .StatusDescription}}</p>
 {{end}}


Test patch:
diff --git a/policy/policy_test.go b/policy/policy_test.go
index 61fdd6d..94333f8 100644
--- a/policy/policy_test.go
+++ b/policy/policy_test.go
@@ -47,8 +47,8 @@ func TestEvaluator(t *testing.T) {
 				Status: common.StatusApproved,
 			},
 			disapproval: &StaticEvaluator{
-				Status:      common.StatusDisapproved,
-				Description: "disapproved by test",
+				Status:            common.StatusDisapproved,
+				StatusDescription: "disapproved by test",
 			},
 		}
 
@@ -56,14 +56,14 @@ func TestEvaluator(t *testing.T) {
 		require.NoError(t, r.Error)
 
 		assert.Equal(t, common.StatusDisapproved, r.Status)
-		assert.Equal(t, "disapproved by test", r.Description)
+		assert.Equal(t, "disapproved by test", r.StatusDescription)
 	})
 
 	t.Run("approvalWinsByDefault", func(t *testing.T) {
 		eval := evaluator{
 			approval: &StaticEvaluator{
-				Status:      common.StatusPending,
-				Description: "2 approvals needed",
+				Status:            common.StatusPending,
+				StatusDescription: "2 approvals needed",
 			},
 			disapproval: &StaticEvaluator{
 				Status: common.StatusSkipped,
@@ -74,7 +74,7 @@ func TestEvaluator(t *testing.T) {
 		require.NoError(t, r.Error)
 
 		assert.Equal(t, common.StatusPending, r.Status)
-		assert.Equal(t, "2 approvals needed", r.Description)
+		assert.Equal(t, "2 approvals needed", r.StatusDescription)
 	})
 
 	t.Run("propagateError", func(t *testing.T) {
diff --git a/policy/reviewer/reviewer_test.go b/policy/reviewer/reviewer_test.go
index 23e1277..fd54e19 100644
--- a/policy/reviewer/reviewer_test.go
+++ b/policy/reviewer/reviewer_test.go
@@ -31,11 +31,12 @@ import (
 
 func TestFindLeafResults(t *testing.T) {
 	result := makeResult(&common.Result{
-		Name:        "Skipped",
-		Description: "",
-		Status:      common.StatusSkipped,
-		Error:       nil,
-		Children:    nil,
+		Name:              "Skipped",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusSkipped,
+		Error:             nil,
+		Children:          nil,
 	}, "random-users")
 	actualResults := FindRequests(result)
 	require.Len(t, actualResults, 2, "incorrect number of leaf results")
@@ -171,9 +172,10 @@ func TestFindRepositoryCollaborators(t *testing.T) {
 func TestSelectReviewers(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Owner",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Owner",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
@@ -197,9 +199,10 @@ func TestSelectReviewers(t *testing.T) {
 func TestSelectAdminTeam(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Owner",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Owner",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Admins:        true,
 			RequiredCount: 1,
@@ -222,9 +225,10 @@ func TestSelectAdminTeam(t *testing.T) {
 func TestSelectReviewers_Team(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write"},
@@ -249,9 +253,10 @@ func TestSelectReviewers_Team(t *testing.T) {
 func TestSelectReviewers_Team_teams(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-write", "everyone/team-not-collaborators"},
@@ -278,9 +283,10 @@ func TestSelectReviewers_Team_teams(t *testing.T) {
 func TestSelectReviewers_Team_teamsDefaultsToNothing(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require a team approval
 			Teams:         []string{"everyone/team-not-collaborators"},
@@ -302,9 +308,10 @@ func TestSelectReviewers_Team_teamsDefaultsToNothing(t *testing.T) {
 func TestSelectReviewers_Org(t *testing.T) {
 	r := rand.New(rand.NewSource(42))
 	results := makeResults(&common.Result{
-		Name:        "Team",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "Team",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			// Require everyone org approval
 			Organizations: []string{"everyone"},
@@ -330,9 +337,10 @@ func makeResults(result *common.Result, mode string) []*common.Result {
 
 func makeResult(result *common.Result, mode string) *common.Result {
 	return &common.Result{
-		Name:        "One",
-		Description: "",
-		Status:      common.StatusPending,
+		Name:              "One",
+		Description:       "",
+		StatusDescription: "",
+		Status:            common.StatusPending,
 		ReviewRequestRule: &common.ReviewRequestRule{
 			Users:         []string{"neverappears"},
 			RequiredCount: 0,
@@ -341,9 +349,10 @@ func makeResult(result *common.Result, mode string) *common.Result {
 		Error: nil,
 		Children: []*common.Result{
 			{
-				Name:        "Two",
-				Description: "",
-				Status:      common.StatusPending,
+				Name:              "Two",
+				Description:       "",
+				StatusDescription: "",
+				Status:            common.StatusPending,
 				ReviewRequestRule: &common.ReviewRequestRule{
 					Users:         []string{"mhaypenny", "review-approver"},
 					RequiredCount: 1,
@@ -356,6 +365,7 @@ func makeResult(result *common.Result, mode string) *common.Result {
 			{
 				Name:              "Three",
 				Description:       "",
+				StatusDescription: "",
 				Status:            common.StatusDisapproved,
 				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             errors.New("foo"),
@@ -364,14 +374,16 @@ func makeResult(result *common.Result, mode string) *common.Result {
 			{
 				Name:              "Four",
 				Description:       "",
+				StatusDescription: "",
 				Status:            common.StatusPending,
 				ReviewRequestRule: &common.ReviewRequestRule{},
 				Error:             nil,
 				Children: []*common.Result{
 					{
-						Name:        "Five",
-						Description: "",
-						Status:      common.StatusPending,
+						Name:              "Five",
+						Description:       "",
+						StatusDescription: "",
+						Status:            common.StatusPending,
 						ReviewRequestRule: &common.ReviewRequestRule{
 							Users:              []string{"contributor-committer", "contributor-author", "not-a-collaborator"},
 							RequiredCount:      1,