diff --git a/config/policy-examples/complicated.yml b/config/policy-examples/complicated.yml index 5ee7528f..65e8d5de 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 db6a5a8d..6c6eca0f 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 abd02ba3..9e0e144c 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 8accdbd6..71c6bc01 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 54b41a87..64f1ed12 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 eedb7577..a1e436e4 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/policy/policy_test.go b/policy/policy_test.go index 61fdd6d4..94333f80 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 23e12775..fd54e199 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, diff --git a/server/handler/base.go b/server/handler/base.go index b5d7f73c..9d297cfd 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 8056f3d3..62e4a0c6 100644 --- a/server/templates/details.html.tmpl +++ b/server/templates/details.html.tmpl @@ -31,7 +31,7 @@

Status: {{$s | titlecase}}

-

{{or .Result.Error .Result.Description}}

+

{{or .Result.Error .Result.StatusDescription}}

@@ -68,5 +68,8 @@ {{.Name}} {{$s | titlecase}}

-

{{or .Error .Description}}

+ {{if (and .Description (ne $s "skipped"))}} +

{{.Description}}

+ {{end}} +

{{or .Error .StatusDescription}}

{{end}}