From 14959a4577d2b21e1ab7323a49e9d28eaac51f7c Mon Sep 17 00:00:00 2001 From: Rory Doherty Date: Thu, 8 Feb 2024 15:03:39 +0000 Subject: [PATCH] Add Option to not post status to github on failures and add equals operator for modifications --- README.md | 56 +++++++++++++++------------ policy/policy.go | 5 +++ policy/predicate/file.go | 7 ++++ policy/predicate/file_test.go | 70 +++++++++++++++++++++++++++++++++- server/handler/eval_context.go | 5 +++ 5 files changed, 117 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 5165c35ff..bb40b2935 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# policy-bot +# policy-bot [![Docker Pulls](https://img.shields.io/docker/pulls/palantirtechnologies/policy-bot.svg)](https://hub.docker.com/r/palantirtechnologies/policy-bot/) @@ -20,23 +20,24 @@ UI to view the detailed approval status of any pull request. [required status check]: https://help.github.com/articles/enabling-required-status-checks/ [required reviews]: https://help.github.com/articles/about-required-reviews-for-pull-requests/ -* [Configuration](#configuration) - + [policy.yml Specification](#policyyml-specification) - + [Approval Rules](#approval-rules) - + [Approval Policies](#approval-policies) - + [Disapproval Policy](#disapproval-policy) - + [Caveats and Notes](#caveats-and-notes) +- [Configuration](#configuration) + - [policy.yml Specification](#policyyml-specification) + - [Approval Rules](#approval-rules) + - [Approval Policies](#approval-policies) + - [Disapproval Policy](#disapproval-policy) + - [Testing and Debugging Policies](#testing-and-debugging-policies) + - [Caveats and Notes](#caveats-and-notes) - [Disapproval is Disabled by Default](#disapproval-is-disabled-by-default) - [Interactions with GitHub Reviews](#interactions-with-github-reviews) - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates) - [Cross-organization Membership Tests](#cross-organization-membership-tests) - [Update Merges](#update-merges) - [Automatically Requesting Reviewers](#automatically-requesting-reviewers) -* [Security](#security) -* [Deployment](#deployment) -* [Development](#development) -* [Contributing](#contributing) -* [License](#license) +- [Security](#security) +- [Deployment](#deployment) +- [Development](#development) +- [Contributing](#contributing) +- [License](#license) ## Configuration @@ -97,9 +98,14 @@ approval_rules: - "^staging/.*$" requires: count: 0 + +# Settings which affect how policy-bot acts +settings: + # Set the below to true if you only want policy-bot statuses on PRs where the policy has evaluated to true + only_post_success_status: false ``` -#### Notes on YAML Syntax +#### Notes on YAML Syntax The YAML language specification supports flow scalars (basic values like strings and numbers) in three formats: @@ -117,7 +123,7 @@ escape characters, which can cause confusion when used for regex strings - Plain: There are no escape characters. Backslash characters do not need to be escaped. e.g. `^BREAKING CHANGE: (\w| )+$` -#### Remote Policy Configuration +#### Remote Policy Configuration You can also define a remote policy by specifying a repository, path, and ref (only repository is required). Instead of defining a `policy` key, you would @@ -228,7 +234,7 @@ if: # "modified_lines" is satisfied if the number of lines added or deleted by # the pull request matches any of the listed conditions. Each expression is - # an operator (one of '<' or '>'), an optional space, and a number. + # an operator (one of '<', '>' or '='), an optional space, and a number. modified_lines: additions: "> 100" deletions: "> 100" @@ -638,7 +644,7 @@ repository `admin` permissions as organization owners are not selected. The `teams` mode needs the team visibility to be set to `visibile` to enable this functionality for a given team. -##### Example +##### Example Given the following example requirement rule, @@ -660,7 +666,7 @@ set of users of in Where the Pull Request Author and any non direct collaborators have been removed from the set. -#### Invalidating Approval on Push +#### Invalidating Approval on Push By default, `policy-bot` does not invalidate exisitng approvals when users add new commits to a pull request. You can control this behavior for each rule in a @@ -688,7 +694,7 @@ in mid-2023 because computing it was unreliable and inaccurate (see issue [#598]: https://github.com/palantir/policy-bot/issues/598 -#### Expanding Required Reviewers +#### Expanding Required Reviewers The details view for a pull request shows the users, organizations, teams, and permission levels that are reqired to approve each rule. When the @@ -715,7 +721,7 @@ While `policy-bot` can be used to implement security controls on GitHub repositories, there are important limitations to be aware of before adopting this approach. -### Status Checks +### Status Checks `policy-bot` reports approval status to GitHub using [commit statuses][]. While statuses cannot be deleted, they can be set or overwritten by any user with @@ -732,7 +738,7 @@ approve and merge a pull request before `policy-bot` can detect the problem. Organizations concerned about this case should monitor and alert on the relevant audit logs or minimize write access to repositories. -### Comment Edits +### Comment Edits GitHub users with sufficient permissions can edit the comments of other users, possibly changing an unrelated comment into one that enables approval. @@ -745,7 +751,7 @@ logs. This issue can also be minimized by only using GitHub reviews for approval, at the expense of removing the ability to self-approve pull requests. -### Commit Users +### Commit Users GitHub associates commits with users by mapping the email address in a commit to email addresses associated with GitHub user accounts. `policy-bot` then uses @@ -762,7 +768,7 @@ failure modes in this process: If using GitHub Enterprise, both of these issues are avoidable by using the [commit-current-user-check][] pre-receive hook. -### Update Merge Conflicts +### Update Merge Conflicts When using the `ignore_update_merges` option, `policy-bot` cannot tell the difference between clean merges and merges that contain conflict resolution. @@ -796,7 +802,7 @@ variables for server values are prefixed with `POLICYBOT_` (e.g. `POLICYBOT_PORT`). This prefix can be overridden by setting the `POLICYBOT_ENV_PREFIX` environment variable. -### GitHub App Configuration +### GitHub App Configuration To configure `policy-bot` as a GitHub App, set these options in GitHub: @@ -843,7 +849,7 @@ generated values: - Client secret (`github.oauth.client_secret`) - Private key (`github.app.private_key`) -### Operations +### Operations `policy-bot` uses [go-baseapp](https://github.com/palantir/go-baseapp) and [go-githubapp](https://github.com/palantir/go-githubapp), both of which emit @@ -904,7 +910,7 @@ and [Yarn](https://yarnpkg.com/). modified config file `policy-bot.yml` - The server is available at `http://localhost:8080/` -### Example Policy Files +### Example Policy Files Example policy files can be found in [`config/policy-examples`](https://github.com/palantir/policy-bot/tree/develop/config/policy-examples) diff --git a/policy/policy.go b/policy/policy.go index d7c01f4f8..b6be0cb7f 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -37,6 +37,11 @@ type RemoteConfig struct { type Config struct { Policy Policy `yaml:"policy"` ApprovalRules []*approval.Rule `yaml:"approval_rules"` + Settings Settings `yaml:"settings"` +} + +type Settings struct { + OnlyPostSuccessStatus bool `yaml:"only_post_success_status"` } type Policy struct { diff --git a/policy/predicate/file.go b/policy/predicate/file.go index d325429ce..8e7934020 100644 --- a/policy/predicate/file.go +++ b/policy/predicate/file.go @@ -153,6 +153,7 @@ const ( OpNone CompareOp = iota OpLessThan OpGreaterThan + OpEquals ) type ComparisonExpr struct { @@ -170,6 +171,8 @@ func (exp ComparisonExpr) Evaluate(n int64) bool { return n < exp.Value case OpGreaterThan: return n > exp.Value + case OpEquals: + return n == exp.Value } return false } @@ -185,6 +188,8 @@ func (exp ComparisonExpr) MarshalText() ([]byte, error) { op = "<" case OpGreaterThan: op = ">" + case OpEquals: + op = "=" default: return nil, errors.Errorf("unknown operation: %d", exp.Op) } @@ -213,6 +218,8 @@ func (exp *ComparisonExpr) UnmarshalText(text []byte) error { op = OpLessThan case '>': op = OpGreaterThan + case '=': + op = OpEquals default: return errors.Errorf("invalid comparison operator: %c", text[i]) } diff --git a/policy/predicate/file_test.go b/policy/predicate/file_test.go index aed8159af..ef4c2ebc3 100644 --- a/policy/predicate/file_test.go +++ b/policy/predicate/file_test.go @@ -297,6 +297,70 @@ func TestModifiedLines(t *testing.T) { }, }, }) + + p = &ModifiedLines{ + Total: ComparisonExpr{Op: OpEquals, Value: 100}, + } + + runFileTests(t, p, []FileTestCase{ + { + "total", + []*pull.File{ + {Additions: 20, Deletions: 20}, + {Additions: 20}, + {Additions: 20, Deletions: 20}, + }, + &common.PredicateResult{ + Satisfied: true, + Values: []string{"total 100"}, + ConditionValues: []string{"total modifications = 100"}, + }, + }, + }) + + p = &ModifiedLines{ + Additions: ComparisonExpr{Op: OpEquals, Value: 100}, + Deletions: ComparisonExpr{Op: OpEquals, Value: 25}, + } + + runFileTests(t, p, []FileTestCase{ + { + "empty", + []*pull.File{}, + &common.PredicateResult{ + Satisfied: false, + Values: []string{"+0", "-0"}, + ConditionValues: []string{"added lines = 100", "deleted lines = 25"}, + }, + }, + { + "additions", + []*pull.File{ + {Additions: 55}, + {Additions: 45}, + }, + &common.PredicateResult{ + Satisfied: true, + Values: []string{"+100"}, + ConditionValues: []string{"added lines = 100"}, + }, + }, + { + "deletions", + []*pull.File{ + {Additions: 5, Deletions: 5}, + {Deletions: 10}, + {Additions: 5}, + {Deletions: 10}, + }, + &common.PredicateResult{ + Satisfied: true, + Values: []string{"-25"}, + ConditionValues: []string{"deleted lines = 25"}, + }, + }, + }) + } func TestComparisonExpr(t *testing.T) { @@ -352,6 +416,10 @@ func TestComparisonExpr(t *testing.T) { Input: ">100", Output: ComparisonExpr{Op: OpGreaterThan, Value: 100}, }, + "equals": { + Input: "=100", + Output: ComparisonExpr{Op: OpEquals, Value: 100}, + }, "innerSpaces": { Input: "< 35", Output: ComparisonExpr{Op: OpLessThan, Value: 35}, @@ -365,7 +433,7 @@ func TestComparisonExpr(t *testing.T) { Output: ComparisonExpr{Op: OpLessThan, Value: 35}, }, "invalidOp": { - Input: "=10", + Input: "~10", Err: true, }, "invalidValue": { diff --git a/server/handler/eval_context.go b/server/handler/eval_context.go index 4f9a8995a..2c7daf976 100644 --- a/server/handler/eval_context.go +++ b/server/handler/eval_context.go @@ -195,6 +195,11 @@ func (ec *EvalContext) PostStatus(ctx context.Context, state, message string) { return } + if state != "success" && ec.Config.Config.Settings.OnlyPostSuccessStatus { + logger.Info().Msg("Skipping status update as it is not success and the setting is enabled to only post status updates on success") + return + } + if !ec.PullContext.IsOpen() { logger.Info().Msg("Skipping status update because PR state is not open") return