feat(policy): Support marshalling of config

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.
palantir · Jul 9, 2024 · 6ad96ce · 6ad96ce
1 parent beb44ef
commit 6ad96ce
Show file tree

Hide file tree

Showing 12 changed files with 223 additions and 69 deletions.
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
@@ -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 {

diff --git a/policy/common/actor.go b/policy/common/actor.go
@@ -26,17 +26,17 @@ 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.
@@ -45,6 +45,10 @@ func (a *Actors) IsEmpty() bool {
 		len(a.Permissions) == 0 && !a.Admins && !a.WriteCollaborators)
 }
 
+func (a *Actors) IsZero() bool {
+	return a.IsEmpty()
+}
+
 // GetPermissions returns unique permissions ordered from most to least
 // permissive. It includes the permissions from the deprecated Admins and
 // WriteCollaborators fields.

diff --git a/policy/common/regexp.go b/policy/common/regexp.go
@@ -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 {

diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go
@@ -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 {

diff --git a/policy/policy.go b/policy/policy.go
@@ -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) {

diff --git a/policy/policy_test.go b/policy/policy_test.go
@@ -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
@@ -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))
 }
diff --git a/policy/predicate/branch.go b/policy/predicate/branch.go
@@ -23,7 +23,7 @@ import (
 )
 
 type TargetsBranch struct {
-	Pattern common.Regexp `yaml:"pattern"`
+	Pattern common.Regexp `yaml:"pattern,omitempty"`
 }
 
 var _ Predicate = &TargetsBranch{}
@@ -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{}

diff --git a/policy/predicate/file.go b/policy/predicate/file.go
@@ -26,8 +26,8 @@ import (
 )
 
 type ChangedFiles struct {
-	Paths       []common.Regexp `yaml:"paths"`
-	IgnorePaths []common.Regexp `yaml:"ignore"`
+	Paths       []common.Regexp `yaml:"paths,omitempty"`
+	IgnorePaths []common.Regexp `yaml:"ignore,omitempty"`
 }
 
 var _ Predicate = &ChangedFiles{}
@@ -86,7 +86,7 @@ func (pred *ChangedFiles) Trigger() common.Trigger {
 }
 
 type OnlyChangedFiles struct {
-	Paths []common.Regexp `yaml:"paths"`
+	Paths []common.Regexp `yaml:"paths,omitempty"`
 }
 
 var _ Predicate = &OnlyChangedFiles{}
@@ -142,8 +142,8 @@ func (pred *OnlyChangedFiles) Trigger() common.Trigger {
 }
 
 type NoChangedFiles struct {
-	Paths       []common.Regexp `yaml:"paths"`
-	IgnorePaths []common.Regexp `yaml:"ignore"`
+	Paths       []common.Regexp `yaml:"paths,omitempty"`
+	IgnorePaths []common.Regexp `yaml:"ignore,omitempty"`
 }
 
 var _ Predicate = &NoChangedFiles{}
@@ -182,9 +182,9 @@ func (pred *NoChangedFiles) Trigger() common.Trigger {
 }
 
 type ModifiedLines struct {
-	Additions ComparisonExpr `yaml:"additions"`
-	Deletions ComparisonExpr `yaml:"deletions"`
-	Total     ComparisonExpr `yaml:"total"`
+	Additions ComparisonExpr `yaml:"additions,omitempty"`
+	Deletions ComparisonExpr `yaml:"deletions,omitempty"`
+	Total     ComparisonExpr `yaml:"total,omitempty"`
 }
 
 type CompareOp uint8