Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a server option to avoid loading pushed dates #599

Merged
merged 3 commits into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,23 @@ issue by using the `ignore_commits_by` option in combination with the
[requirement on a protected branch]: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#require-status-checks-before-merging
[commit-current-user-check]: https://github.com/github/platform-samples/blob/master/pre-receive-hooks/commit-current-user-check.sh

### Invalidate On Push

`policy-bot` can be configured to invalidate approvals when a commit is pushed using
the `invalidate_on_push` option. This option is disabled by default.

However, in recent versions of GitHub after [2023-07-01 breaking changes][], policy-bot
is unable to load pushedDate for commits. This means that it is unable to determine
whether a commit was pushed after the approval was granted.

A server option (`do_not_load_commit_pushed_date`) is provided to toggle loading the push
date for commits. If enabled, policy-bot will not be able to determine whether a commit
was pushed after an approval was granted.

Can also be configured by using the following env variable `POLICYBOT_OPTIONS_DO_NOT_LOAD_COMMIT_PUSHED_DATE`.

[2023-07-01 breaking changes]: https://docs.github.com/en/graphql/overview/breaking-changes#changes-scheduled-for-2023-07-01

## Deployment

`policy-bot` is easy to deploy in your own environment as it has no dependencies
Expand Down
11 changes: 9 additions & 2 deletions pull/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ type GitHubContext struct {
client *github.Client
v4client *githubv4.Client

doNotLoadCommitPushedDate bool

owner string
repo string
number int
Expand All @@ -145,7 +147,7 @@ type GitHubContext struct {
// obtain information. It caches responses for the lifetime of the context. The
// pull request passed to the context must contain at least the base repository
// and the number or the function panics.
func NewGitHubContext(ctx context.Context, mbrCtx MembershipContext, client *github.Client, v4client *githubv4.Client, loc Locator) (Context, error) {
func NewGitHubContext(ctx context.Context, mbrCtx MembershipContext, client *github.Client, v4client *githubv4.Client, loc Locator, doNotLoadCommitPushedDate bool) (Context, error) {
if loc.Owner == "" || loc.Repo == "" || loc.Number == 0 {
panic("pull request object does not contain full identifying information")
}
Expand All @@ -166,6 +168,8 @@ func NewGitHubContext(ctx context.Context, mbrCtx MembershipContext, client *git
repo: loc.Repo,
number: loc.Number,
pr: pr,

doNotLoadCommitPushedDate: doNotLoadCommitPushedDate,
}, nil
}

Expand Down Expand Up @@ -814,6 +818,9 @@ func (ghc *GitHubContext) loadCommits() ([]*Commit, error) {
if err != nil {
return nil, err
}
if ghc.doNotLoadCommitPushedDate {
return commits, nil
}
if head.PushedAt != nil {
return commits, nil
}
Expand Down Expand Up @@ -862,7 +869,7 @@ func (ghc *GitHubContext) loadCommitsOnce() (head *Commit, commits []*Commit, er
// In the second case, retrying after a delay can fix things, but the delay
// can be 15+ seconds in practice, so using the alternate API should
// improve latency at the cost of more API requests.
if head.PushedAt == nil {
if head.PushedAt == nil && !ghc.doNotLoadCommitPushedDate {
log.Debug().
Bool("fork", ghc.pr.IsCrossRepository).
Msgf("failed to load pushed date via pull request, falling back to commit APIs")
Expand Down
2 changes: 1 addition & 1 deletion pull/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ func makeContext(t *testing.T, rp *ResponsePlayer, pr *github.PullRequest) Conte
Repo: pr.GetBase().GetRepo().GetName(),
Number: pr.GetNumber(),
Value: pr,
})
}, false)
require.NoError(t, err, "failed to create github context")

return prctx
Expand Down
2 changes: 1 addition & 1 deletion server/handler/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (b *Base) NewEvalContext(ctx context.Context, installationID int64, loc pul
}

mbrCtx := NewCrossOrgMembershipContext(ctx, client, loc.Owner, b.Installations, b.ClientCreator)
prctx, err := pull.NewGitHubContext(ctx, mbrCtx, client, v4client, loc)
prctx, err := pull.NewGitHubContext(ctx, mbrCtx, client, v4client, loc, b.PullOpts.DoNotLoadCommitPushedDate)
if err != nil {
return nil, err
}
Expand Down
17 changes: 16 additions & 1 deletion server/handler/eval_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package handler

import (
"os"
"strconv"
)

const (
Expand Down Expand Up @@ -45,6 +46,11 @@ type PullEvaluationOptions struct {
//
// TODO(bkeyes): remove in version 2.0
Deprecated_AppName string `yaml:"app_name"`

// As of 2023-07-01 the commit.pushedDate graphql field is removed from GitHub.
// !Warning!
// Setting this option effectively breaks all usage of the invalidate_on_push approval rule
DoNotLoadCommitPushedDate bool `yaml:"do_not_load_commit_pushed_date"`
asvoboda marked this conversation as resolved.
Show resolved Hide resolved
}

func (p *PullEvaluationOptions) fillDefaults() {
Expand All @@ -57,7 +63,6 @@ func (p *PullEvaluationOptions) fillDefaults() {
if p.SharedPolicyPath == "" {
p.SharedPolicyPath = DefaultSharedPolicyPath
}

if p.StatusCheckContext == "" {
p.StatusCheckContext = DefaultStatusCheckContext
}
Expand All @@ -68,6 +73,7 @@ func (p *PullEvaluationOptions) SetValuesFromEnv(prefix string) {
setStringFromEnv("SHARED_REPOSITORY", prefix, &p.SharedRepository)
setStringFromEnv("SHARED_POLICY_PATH", prefix, &p.SharedPolicyPath)
setStringFromEnv("STATUS_CHECK_CONTEXT", prefix, &p.StatusCheckContext)
setBoolFromEnv("DO_NOT_LOAD_COMMIT_PUSHED_DATE", prefix, &p.DoNotLoadCommitPushedDate)
p.fillDefaults()
}

Expand All @@ -78,3 +84,12 @@ func setStringFromEnv(key, prefix string, value *string) bool {
}
return false
}

func setBoolFromEnv(key, prefix string, value *bool) bool {
if v, ok := os.LookupEnv(prefix + key); ok {
parsedResult, _ := strconv.ParseBool(v)
*value = parsedResult
return true
}
return false
}
Loading