Skip to content

Commit

Permalink
feat(policy): Support marshalling of config
Browse files Browse the repository at this point in the history
Sometimes it's convenient to generate a Policy Bot config. For example,
when handling required but optional workflows, you need the config file to
contain a rule for each workflow which replicates its path filters.

That job is made much easier if the Policy Bot types can be reused. But
without `omitempty` the generated YAML contains all fields and it's hard
to read/review.

Add `omitempty` where it's needed, and `IsZero` in a few places too -
which is how the behaviour of `omitempty` can be customised.
  • Loading branch information
iainlane committed Sep 4, 2024
1 parent f869ff9 commit 787b1cf
Show file tree
Hide file tree
Showing 15 changed files with 236 additions and 86 deletions.
40 changes: 20 additions & 20 deletions policy/approval/approve.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,32 +29,32 @@ import (
)

type Rule struct {
Name string `yaml:"name"`
Description string `yaml:"description"`
Predicates predicate.Predicates `yaml:"if"`
Options Options `yaml:"options"`
Requires Requires `yaml:"requires"`
Name string `yaml:"name,omitempty"`
Description string `yaml:"description,omitempty"`
Predicates predicate.Predicates `yaml:"if,omitempty"`
Options Options `yaml:"options,omitempty"`
Requires Requires `yaml:"requires,omitempty"`
}

type Options struct {
AllowAuthor bool `yaml:"allow_author"`
AllowContributor bool `yaml:"allow_contributor"`
AllowNonAuthorContributor bool `yaml:"allow_non_author_contributor"`
InvalidateOnPush bool `yaml:"invalidate_on_push"`
AllowAuthor bool `yaml:"allow_author,omitempty"`
AllowContributor bool `yaml:"allow_contributor,omitempty"`
AllowNonAuthorContributor bool `yaml:"allow_non_author_contributor,omitempty"`
InvalidateOnPush bool `yaml:"invalidate_on_push,omitempty"`

IgnoreEditedComments bool `yaml:"ignore_edited_comments"`
IgnoreUpdateMerges bool `yaml:"ignore_update_merges"`
IgnoreCommitsBy common.Actors `yaml:"ignore_commits_by"`
IgnoreEditedComments bool `yaml:"ignore_edited_comments,omitempty"`
IgnoreUpdateMerges bool `yaml:"ignore_update_merges,omitempty"`
IgnoreCommitsBy common.Actors `yaml:"ignore_commits_by,omitempty"`

RequestReview RequestReview `yaml:"request_review"`
RequestReview RequestReview `yaml:"request_review,omitempty"`

Methods *common.Methods `yaml:"methods"`
Methods *common.Methods `yaml:"methods,omitempty"`
}

type RequestReview struct {
Enabled bool `yaml:"enabled"`
Mode common.RequestMode `yaml:"mode"`
Count int `yaml:"count"`
Enabled bool `yaml:"enabled,omitempty"`
Mode common.RequestMode `yaml:"mode,omitempty"`
Count int `yaml:"count,omitempty"`
}

func (opts *Options) GetMethods() *common.Methods {
Expand All @@ -78,9 +78,9 @@ func (opts *Options) GetMethods() *common.Methods {
}

type Requires struct {
Count int `yaml:"count"`
Count int `yaml:"count,omitempty"`
Actors common.Actors `yaml:",inline"`
Conditions predicate.Predicates `yaml:"conditions"`
Conditions predicate.Predicates `yaml:"conditions,omitempty"`
}

func (r *Rule) Trigger() common.Trigger {
Expand Down Expand Up @@ -411,7 +411,7 @@ func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull
commits = sortCommits(commits, prctx.HeadSHA())

ignoreUpdates := r.Options.IgnoreUpdateMerges
ignoreCommits := !r.Options.IgnoreCommitsBy.IsEmpty()
ignoreCommits := !r.Options.IgnoreCommitsBy.IsZero()

if !ignoreUpdates && !ignoreCommits {
return commits, nil
Expand Down
16 changes: 8 additions & 8 deletions policy/common/actor.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,21 @@ import (
// team and organization memberships. The set of allowed actors is the union of
// all conditions in this structure.
type Actors struct {
Users []string `yaml:"users" json:"users"`
Teams []string `yaml:"teams" json:"teams"`
Organizations []string `yaml:"organizations" json:"organizations"`
Users []string `yaml:"users,omitempty" json:"users"`
Teams []string `yaml:"teams,omitempty" json:"teams"`
Organizations []string `yaml:"organizations,omitempty" json:"organizations"`

// Deprecated: use Permissions with "admin" or "write"
Admins bool `yaml:"admins" json:"-"`
WriteCollaborators bool `yaml:"write_collaborators" json:"-"`
Admins bool `yaml:"admins,omitempty" json:"-"`
WriteCollaborators bool `yaml:"write_collaborators,omitempty" json:"-"`

// A list of GitHub collaborator permissions that are allowed. Values may
// be any of "admin", "maintain", "write", "triage", and "read".
Permissions []pull.Permission `yaml:"permissions" json:"permissions"`
Permissions []pull.Permission `yaml:"permissions,omitempty" json:"permissions"`
}

// IsEmpty returns true if no conditions for actors are defined.
func (a *Actors) IsEmpty() bool {
// IsZero returns true if no conditions for actors are defined.
func (a *Actors) IsZero() bool {
return a == nil || (len(a.Users) == 0 && len(a.Teams) == 0 && len(a.Organizations) == 0 &&
len(a.Permissions) == 0 && !a.Admins && !a.WriteCollaborators)
}
Expand Down
10 changes: 5 additions & 5 deletions policy/common/actor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,17 @@ func TestIsActor(t *testing.T) {

func TestIsEmpty(t *testing.T) {
a := &Actors{}
assert.True(t, a.IsEmpty(), "Actors struct was not empty")
assert.True(t, a.IsZero(), "Actors struct was not empty")

a = &Actors{Users: []string{"user"}}
assert.False(t, a.IsEmpty(), "Actors struct was empty")
assert.False(t, a.IsZero(), "Actors struct was empty")

a = &Actors{Teams: []string{"org/team"}}
assert.False(t, a.IsEmpty(), "Actors struct was empty")
assert.False(t, a.IsZero(), "Actors struct was empty")

a = &Actors{Organizations: []string{"org"}}
assert.False(t, a.IsEmpty(), "Actors struct was empty")
assert.False(t, a.IsZero(), "Actors struct was empty")

a = nil
assert.True(t, a.IsEmpty(), "nil struct was not empty")
assert.True(t, a.IsZero(), "nil struct was not empty")
}
8 changes: 8 additions & 0 deletions policy/common/regexp.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ type Regexp struct {
r *regexp.Regexp
}

func (r Regexp) MarshalYAML() (interface{}, error) {
return r.String(), nil
}

func (r Regexp) IsZero() bool {
return r.r == nil
}

func NewRegexp(pattern string) (Regexp, error) {
r, err := regexp.Compile(pattern)
if err != nil {
Expand Down
24 changes: 16 additions & 8 deletions policy/disapproval/disapprove.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,26 @@ import (
)

type Policy struct {
Predicates predicate.Predicates `yaml:"if"`
Options Options `yaml:"options"`
Requires Requires `yaml:"requires"`
Predicates predicate.Predicates `yaml:"if,omitempty"`
Options Options `yaml:"options,omitempty"`
Requires Requires `yaml:"requires,omitempty"`
}

type Options struct {
Methods Methods `yaml:"methods"`
Methods Methods `yaml:"methods,omitempty"`
}

func (opts Options) IsZero() bool {
return opts.Methods.IsZero()
}

type Methods struct {
Disapprove *common.Methods `yaml:"disapprove"`
Revoke *common.Methods `yaml:"revoke"`
Disapprove *common.Methods `yaml:"disapprove,omitempty"`
Revoke *common.Methods `yaml:"revoke,omitempty"`
}

func (m *Methods) IsZero() bool {
return m.Disapprove == nil && m.Revoke == nil
}

func (opts *Options) GetDisapproveMethods() *common.Methods {
Expand Down Expand Up @@ -82,7 +90,7 @@ type Requires struct {
func (p *Policy) Trigger() common.Trigger {
t := common.TriggerCommit

if !p.Requires.IsEmpty() {
if !p.Requires.IsZero() {
dm := p.Options.GetDisapproveMethods()
rm := p.Options.GetRevokeMethods()

Expand Down Expand Up @@ -136,7 +144,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R
}
}
res.PredicateResults = predicateResults
if p.Requires.IsEmpty() {
if p.Requires.IsZero() {
log.Debug().Msg("no users are allowed to disapprove; skipping")

res.StatusDescription = "No disapproval policy is specified or the policy is empty"
Expand Down
14 changes: 7 additions & 7 deletions policy/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ import (
// with the default value being the configured default policy file location. The Ref is optional,
// and the default branch of the Remote repository will be used.
type RemoteConfig struct {
Remote string `yaml:"remote"`
Path string `yaml:"path"`
Ref string `yaml:"ref"`
Remote string `yaml:"remote,omitempty"`
Path string `yaml:"path,omitempty"`
Ref string `yaml:"ref,omitempty"`
}

type Config struct {
Policy Policy `yaml:"policy"`
ApprovalRules []*approval.Rule `yaml:"approval_rules"`
Policy Policy `yaml:"policy,omitempty"`
ApprovalRules []*approval.Rule `yaml:"approval_rules,omitempty"`
}

type Policy struct {
Approval approval.Policy `yaml:"approval"`
Disapproval *disapproval.Policy `yaml:"disapproval"`
Approval approval.Policy `yaml:"approval,omitempty"`
Disapproval *disapproval.Policy `yaml:"disapproval,omitempty"`
}

func ParsePolicy(c *Config) (common.Evaluator, error) {
Expand Down
130 changes: 130 additions & 0 deletions policy/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,18 @@ package policy
import (
"context"
"errors"
"regexp"
"testing"

"github.com/palantir/policy-bot/policy/approval"
"github.com/palantir/policy-bot/policy/common"
"github.com/palantir/policy-bot/policy/disapproval"
"github.com/palantir/policy-bot/policy/predicate"
"github.com/palantir/policy-bot/pull"
"github.com/palantir/policy-bot/pull/pulltest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v2"
)

type StaticEvaluator common.Result
Expand Down Expand Up @@ -113,6 +118,131 @@ func TestEvaluator(t *testing.T) {
})
}

func TestConfigMarshalYaml(t *testing.T) {
tests := []struct {
name string
config Config
expected string
}{
{
name: "empty",
config: Config{},
},
{
name: "withDisapproval",
config: Config{
Policy: Policy{
Disapproval: &disapproval.Policy{},
},
},
expected: `policy:
disapproval: {}
`,
},
{
name: "withApprovalRules",
config: Config{
ApprovalRules: []*approval.Rule{
{
Name: "rule1",
},
},
},
expected: `approval_rules:
- name: rule1
`,
},
{
name: "withChangedFiles",
config: Config{
ApprovalRules: []*approval.Rule{
{
Name: "rule1",
Predicates: predicate.Predicates{
ChangedFiles: &predicate.ChangedFiles{
Paths: []common.Regexp{
common.NewCompiledRegexp(regexp.MustCompile(`^\.github/workflows/.*\.yml$`)),
},
},
},
},
},
},
expected: `approval_rules:
- name: rule1
if:
changed_files:
paths:
- ^\.github/workflows/.*\.yml$
`,
},
{
name: "author",
config: Config{
ApprovalRules: []*approval.Rule{
{
Name: "rule1",
Predicates: predicate.Predicates{
HasAuthorIn: &predicate.HasAuthorIn{
Actors: common.Actors{
Users: []string{"author1", "author2"},
},
},
AuthorIsOnlyContributor: new(predicate.AuthorIsOnlyContributor),
},
},
},
},
expected: `approval_rules:
- name: rule1
if:
has_author_in:
users:
- author1
- author2
author_is_only_contributor: false
`,
},
{
name: "modifiedLines",
config: Config{
ApprovalRules: []*approval.Rule{
{
Name: "rule1",
Predicates: predicate.Predicates{
ModifiedLines: &predicate.ModifiedLines{
Additions: predicate.ComparisonExpr{
Op: predicate.OpGreaterThan,
Value: 10,
},
},
},
},
},
},
expected: `approval_rules:
- name: rule1
if:
modified_lines:
additions: '> 10'
`,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
expected := test.expected
if expected == "" {
expected = "{}\n"
}

b, err := yaml.Marshal(test.config)
require.NoError(t, err)
require.Equal(t, expected, string(b))
})
}
}

func castToResult(e common.Evaluator) *common.Result {
return (*common.Result)(e.(*StaticEvaluator))
}
4 changes: 2 additions & 2 deletions policy/predicate/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

type TargetsBranch struct {
Pattern common.Regexp `yaml:"pattern"`
Pattern common.Regexp `yaml:"pattern,omitempty"`
}

var _ Predicate = &TargetsBranch{}
Expand Down Expand Up @@ -54,7 +54,7 @@ func (pred *TargetsBranch) Trigger() common.Trigger {
}

type FromBranch struct {
Pattern common.Regexp `yaml:"pattern"`
Pattern common.Regexp `yaml:"pattern,omitempty"`
}

var _ Predicate = &FromBranch{}
Expand Down
Loading

0 comments on commit 787b1cf

Please sign in to comment.