From 787b1cf600058063316f492d80cc542693bf45ad Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Tue, 9 Jul 2024 18:07:49 +0100 Subject: [PATCH] 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. --- policy/approval/approve.go | 40 ++++----- policy/common/actor.go | 16 ++-- policy/common/actor_test.go | 10 +-- policy/common/regexp.go | 8 ++ policy/disapproval/disapprove.go | 24 +++-- policy/policy.go | 14 +-- policy/policy_test.go | 130 ++++++++++++++++++++++++++++ policy/predicate/branch.go | 4 +- policy/predicate/file.go | 16 ++-- policy/predicate/predicates.go | 42 +++++---- policy/predicate/repository.go | 4 +- policy/predicate/signature.go | 4 +- policy/predicate/status.go | 4 +- policy/predicate/title.go | 4 +- server/handler/details_reviewers.go | 2 +- 15 files changed, 236 insertions(+), 86 deletions(-) diff --git a/policy/approval/approve.go b/policy/approval/approve.go index ba484079..29cd3569 100644 --- 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 { @@ -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 { @@ -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 diff --git a/policy/common/actor.go b/policy/common/actor.go index f72de3a7..cd210d77 100644 --- a/policy/common/actor.go +++ b/policy/common/actor.go @@ -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) } diff --git a/policy/common/actor_test.go b/policy/common/actor_test.go index 5db83f7d..82ccfff1 100644 --- a/policy/common/actor_test.go +++ b/policy/common/actor_test.go @@ -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") } diff --git a/policy/common/regexp.go b/policy/common/regexp.go index 989ee08a..325f106f 100644 --- 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 index e74902e9..b1e0390f 100644 --- 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 { @@ -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() @@ -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" diff --git a/policy/policy.go b/policy/policy.go index d7c01f4f..1d366b15 100644 --- 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 index e9eb4b35..1803447d 100644 --- 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 index 0a68c3fd..c041b5f5 100644 --- 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 index 1f820640..2c5579e2 100644 --- 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 diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go index 8bded609..a01a2a66 100644 --- a/policy/predicate/predicates.go +++ b/policy/predicate/predicates.go @@ -15,36 +15,40 @@ package predicate type Predicates struct { - ChangedFiles *ChangedFiles `yaml:"changed_files"` - NoChangedFiles *NoChangedFiles `yaml:"no_changed_files"` - OnlyChangedFiles *OnlyChangedFiles `yaml:"only_changed_files"` + ChangedFiles *ChangedFiles `yaml:"changed_files,omitempty"` + NoChangedFiles *NoChangedFiles `yaml:"no_changed_files,omitempty"` + OnlyChangedFiles *OnlyChangedFiles `yaml:"only_changed_files,omitempty"` - HasAuthorIn *HasAuthorIn `yaml:"has_author_in"` - HasContributorIn *HasContributorIn `yaml:"has_contributor_in"` - OnlyHasContributorsIn *OnlyHasContributorsIn `yaml:"only_has_contributors_in"` - AuthorIsOnlyContributor *AuthorIsOnlyContributor `yaml:"author_is_only_contributor"` + HasAuthorIn *HasAuthorIn `yaml:"has_author_in,omitempty"` + HasContributorIn *HasContributorIn `yaml:"has_contributor_in,omitempty"` + OnlyHasContributorsIn *OnlyHasContributorsIn `yaml:"only_has_contributors_in,omitempty"` + AuthorIsOnlyContributor *AuthorIsOnlyContributor `yaml:"author_is_only_contributor,omitempty"` - TargetsBranch *TargetsBranch `yaml:"targets_branch"` - FromBranch *FromBranch `yaml:"from_branch"` + TargetsBranch *TargetsBranch `yaml:"targets_branch,omitempty"` + FromBranch *FromBranch `yaml:"from_branch,omitempty"` - ModifiedLines *ModifiedLines `yaml:"modified_lines"` + ModifiedLines *ModifiedLines `yaml:"modified_lines,omitempty"` - HasStatus *HasStatus `yaml:"has_status"` + HasStatus *HasStatus `yaml:"has_status,omitempty"` // `has_successful_status` is a deprecated field that is kept for backwards // compatibility. `has_status` replaces it, and can accept any conclusion // rather than just "success". - HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"` + HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status,omitempty"` - HasWorkflowResult *HasWorkflowResult `yaml:"has_workflow_result"` + HasWorkflowResult *HasWorkflowResult `yaml:"has_workflow_result,omitempty"` - HasLabels *HasLabels `yaml:"has_labels"` + HasLabels *HasLabels `yaml:"has_labels,omitempty"` - Repository *Repository `yaml:"repository"` - Title *Title `yaml:"title"` + Repository *Repository `yaml:"repository,omitempty"` + Title *Title `yaml:"title,omitempty"` - HasValidSignatures *HasValidSignatures `yaml:"has_valid_signatures"` - HasValidSignaturesBy *HasValidSignaturesBy `yaml:"has_valid_signatures_by"` - HasValidSignaturesByKeys *HasValidSignaturesByKeys `yaml:"has_valid_signatures_by_keys"` + HasValidSignatures *HasValidSignatures `yaml:"has_valid_signatures,omitempty"` + HasValidSignaturesBy *HasValidSignaturesBy `yaml:"has_valid_signatures_by,omitempty"` + HasValidSignaturesByKeys *HasValidSignaturesByKeys `yaml:"has_valid_signatures_by_keys,omitempty"` +} + +func (p Predicates) IsZero() bool { + return p == Predicates{} } func (p *Predicates) Predicates() []Predicate { diff --git a/policy/predicate/repository.go b/policy/predicate/repository.go index da8716d1..9603b7d9 100644 --- a/policy/predicate/repository.go +++ b/policy/predicate/repository.go @@ -22,8 +22,8 @@ import ( ) type Repository struct { - Matches []common.Regexp `yaml:"matches"` - NotMatches []common.Regexp `yaml:"not_matches"` + Matches []common.Regexp `yaml:"matches,omitempty"` + NotMatches []common.Regexp `yaml:"not_matches,omitempty"` } var _ Predicate = Repository{} diff --git a/policy/predicate/signature.go b/policy/predicate/signature.go index ce592e9f..959575a8 100644 --- a/policy/predicate/signature.go +++ b/policy/predicate/signature.go @@ -70,7 +70,7 @@ func (pred HasValidSignatures) Trigger() common.Trigger { } type HasValidSignaturesBy struct { - common.Actors `yaml:",inline"` + common.Actors `yaml:",inline,omitempty"` } var _ Predicate = &HasValidSignaturesBy{} @@ -136,7 +136,7 @@ func (pred *HasValidSignaturesBy) Trigger() common.Trigger { } type HasValidSignaturesByKeys struct { - KeyIDs []string `yaml:"key_ids"` + KeyIDs []string `yaml:"key_ids,omitempty"` } var _ Predicate = &HasValidSignaturesByKeys{} diff --git a/policy/predicate/status.go b/policy/predicate/status.go index 9727d9cf..5cdbe459 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -28,8 +28,8 @@ import ( type AllowedConclusions []string type HasStatus struct { - Conclusions AllowedConclusions `yaml:"conclusions"` - Statuses []string `yaml:"statuses"` + Conclusions AllowedConclusions `yaml:"conclusions,omitempty"` + Statuses []string `yaml:"statuses,omitempty"` } func NewHasStatus(statuses []string, conclusions []string) *HasStatus { diff --git a/policy/predicate/title.go b/policy/predicate/title.go index 181714a8..66f2a06d 100644 --- a/policy/predicate/title.go +++ b/policy/predicate/title.go @@ -22,8 +22,8 @@ import ( ) type Title struct { - Matches []common.Regexp `yaml:"matches"` - NotMatches []common.Regexp `yaml:"not_matches"` + Matches []common.Regexp `yaml:"matches,omitempty"` + NotMatches []common.Regexp `yaml:"not_matches,omitempty"` } var _ Predicate = Title{} diff --git a/server/handler/details_reviewers.go b/server/handler/details_reviewers.go index c34e4bf8..1c9c3095 100644 --- a/server/handler/details_reviewers.go +++ b/server/handler/details_reviewers.go @@ -64,7 +64,7 @@ func (h *DetailsReviewers) ServeHTTP(w http.ResponseWriter, r *http.Request) err ruleName := r.URL.Query().Get("rule") requires := findRuleRequires(config.Config, ruleName) - if requires == nil || requires.Count == 0 || requires.Actors.IsEmpty() { + if requires == nil || requires.Count == 0 || requires.Actors.IsZero() { // If the rule does not exist, it does not require approval, or it has // no actors specified, there's no need to list reviewers return h.renderEmptyReviewers(w, r)