From 0bfd9486b8d72b5c9dbdf04d91f09bd1783a48c8 Mon Sep 17 00:00:00 2001 From: Brian John Date: Thu, 8 Apr 2021 16:53:53 -0500 Subject: [PATCH] Update PR when labeled/opened (#235) This will conditionally update a PR whenever the PR is labeled or opened, which will partially fulfill #145. It doesn't seem possible to determine which label was added via the Github webhook payload, so this will update the PR regardless of whether the label added is what is configured in `bulldozer.yml`. Refactor config fetching to happen before calling `ProcessPR` and `UpdatePR` so that it isn't fetched twice during the `pull_request` event when we're both merging and updating a PR. --- README.md | 14 +++-- server/handler/base.go | 77 ++++++++++++++------------- server/handler/check_run.go | 6 ++- server/handler/issue_comment.go | 6 ++- server/handler/pull_request.go | 16 +++++- server/handler/pull_request_review.go | 6 ++- server/handler/push.go | 7 ++- server/handler/status.go | 6 ++- 8 files changed, 89 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 2b53514ce..03eda9663 100644 --- a/README.md +++ b/README.md @@ -236,17 +236,21 @@ which are to be expected, and others that may be caused by mis-configuring Bulld #### Bulldozer isn't updating my branch when it should, what could be happening? -When using the branch update functionality, Bulldozer only acts when the target -branch is updated _after_ updates are enabled for the pull request. For -example: +When using the branch update functionality, Bulldozer only performs an update +_after_ updates are enabled when: +* A label is added +* The pull request is opened +* The target branch is updated + +For example: 1. User A opens a pull request targetting `develop` 2. User B pushes a commit to `develop` -3. User A adds the `update me` label to the first pull request +3. User A adds an `update me` comment to the first pull request 4. User C pushes a commit to `develop` 5. Bulldozer updates the pull request with the commits from Users B and C -Note that the update does _not_ happen when the `update me` label is added, +Note that the update does _not_ happen when the `update me` comment is added, even though there is a new commit on `develop` that is not part of the pull request. diff --git a/server/handler/base.go b/server/handler/base.go index 93e68af66..3b5f7911e 100644 --- a/server/handler/base.go +++ b/server/handler/base.go @@ -33,12 +33,33 @@ type Base struct { PushRestrictionUserToken string } -func (b *Base) ProcessPullRequest(ctx context.Context, pullCtx pull.Context, client *github.Client, pr *github.PullRequest) error { +func (b *Base) FetchConfig(ctx context.Context, client *github.Client, pr *github.PullRequest) (*bulldozer.Config, error) { logger := zerolog.Ctx(ctx) bulldozerConfig, err := b.ConfigForPR(ctx, client, pr) if err != nil { - return errors.Wrap(err, "failed to fetch configuration") + return nil, errors.Wrap(err, "failed to fetch configuration") + } + + switch { + case bulldozerConfig.Missing(): + logger.Debug().Msgf("No configuration found for %s", bulldozerConfig) + return nil, nil + case bulldozerConfig.Invalid(): + logger.Warn().Msgf("Configuration is invalid for %s", bulldozerConfig) + return nil, nil + } + + logger.Debug().Msgf("Found valid configuration for %s", bulldozerConfig) + return bulldozerConfig.Config, nil +} + +func (b *Base) ProcessPullRequest(ctx context.Context, pullCtx pull.Context, client *github.Client, config *bulldozer.Config, pr *github.PullRequest) error { + logger := zerolog.Ctx(ctx) + + if config == nil { + logger.Debug().Msg("ProcessPullRequest: returning immediately due to nil config") + return nil } merger := bulldozer.NewGitHubMerger(client) @@ -50,51 +71,31 @@ func (b *Base) ProcessPullRequest(ctx context.Context, pullCtx pull.Context, cli merger = bulldozer.NewPushRestrictionMerger(merger, bulldozer.NewGitHubMerger(tokenClient)) } - switch { - case bulldozerConfig.Missing(): - logger.Debug().Msgf("No configuration found for %s", bulldozerConfig) - case bulldozerConfig.Invalid(): - logger.Warn().Msgf("Configuration is invalid for %s", bulldozerConfig) - default: - logger.Debug().Msgf("Found valid configuration for %s", bulldozerConfig) - config := *bulldozerConfig.Config - - shouldMerge, err := bulldozer.ShouldMergePR(ctx, pullCtx, config.Merge) - if err != nil { - return errors.Wrap(err, "unable to determine merge status") - } - if shouldMerge { - bulldozer.MergePR(ctx, pullCtx, merger, config.Merge) - } + shouldMerge, err := bulldozer.ShouldMergePR(ctx, pullCtx, config.Merge) + if err != nil { + return errors.Wrap(err, "unable to determine merge status") + } + if shouldMerge { + bulldozer.MergePR(ctx, pullCtx, merger, config.Merge) } return nil } -func (b *Base) UpdatePullRequest(ctx context.Context, pullCtx pull.Context, client *github.Client, pr *github.PullRequest, baseRef string) error { +func (b *Base) UpdatePullRequest(ctx context.Context, pullCtx pull.Context, client *github.Client, config *bulldozer.Config, pr *github.PullRequest, baseRef string) error { logger := zerolog.Ctx(ctx) - bulldozerConfig, err := b.ConfigForPR(ctx, client, pr) - if err != nil { - return errors.Wrap(err, "failed to fetch configuration") + if config == nil { + logger.Debug().Msg("UpdatePullRequest: returning immediately due to nil config") + return nil } - switch { - case bulldozerConfig.Missing(): - logger.Debug().Msgf("No configuration found for %s", bulldozerConfig) - case bulldozerConfig.Invalid(): - logger.Warn().Msgf("Configuration is invalid for %s", bulldozerConfig) - default: - logger.Debug().Msgf("Found valid configuration for %s", bulldozerConfig) - config := *bulldozerConfig.Config - - shouldUpdate, err := bulldozer.ShouldUpdatePR(ctx, pullCtx, config.Update) - if err != nil { - return errors.Wrap(err, "unable to determine update status") - } - if shouldUpdate { - bulldozer.UpdatePR(ctx, pullCtx, client, config.Update, baseRef) - } + shouldUpdate, err := bulldozer.ShouldUpdatePR(ctx, pullCtx, config.Update) + if err != nil { + return errors.Wrap(err, "unable to determine update status") + } + if shouldUpdate { + bulldozer.UpdatePR(ctx, pullCtx, client, config.Update, baseRef) } return nil diff --git a/server/handler/check_run.go b/server/handler/check_run.go index cd7376392..fcd61d082 100644 --- a/server/handler/check_run.go +++ b/server/handler/check_run.go @@ -73,7 +73,11 @@ func (h *CheckRun) Handle(ctx context.Context, eventType, deliveryID string, pay pullCtx := pull.NewGithubContext(client, fullPR) logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() - if err := h.ProcessPullRequest(logger.WithContext(ctx), pullCtx, client, fullPR); err != nil { + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return err + } + if err := h.ProcessPullRequest(logger.WithContext(ctx), pullCtx, client, config, fullPR); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") } } diff --git a/server/handler/issue_comment.go b/server/handler/issue_comment.go index cde82d4e6..e47687694 100644 --- a/server/handler/issue_comment.go +++ b/server/handler/issue_comment.go @@ -57,7 +57,11 @@ func (h *IssueComment) Handle(ctx context.Context, eventType, deliveryID string, } pullCtx := pull.NewGithubContext(client, pr) - if err := h.ProcessPullRequest(ctx, pullCtx, client, pr); err != nil { + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return err + } + if err := h.ProcessPullRequest(ctx, pullCtx, client, config, pr); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") } diff --git a/server/handler/pull_request.go b/server/handler/pull_request.go index e100190de..22a021e01 100644 --- a/server/handler/pull_request.go +++ b/server/handler/pull_request.go @@ -46,6 +46,8 @@ func (h *PullRequest) Handle(ctx context.Context, eventType, deliveryID string, installationID := githubapp.GetInstallationIDFromEvent(&event) ctx, logger := githubapp.PreparePRContext(ctx, installationID, repo, number) + logger.Debug().Msgf("received pull_request %s event", event.GetAction()) + if event.GetAction() == "closed" { logger.Debug().Msg("Doing nothing since pull request is closed") return nil @@ -62,7 +64,19 @@ func (h *PullRequest) Handle(ctx context.Context, eventType, deliveryID string, } pullCtx := pull.NewGithubContext(client, pr) - if err := h.ProcessPullRequest(ctx, pullCtx, client, pr); err != nil { + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return err + } + + if event.GetAction() == "labeled" || event.GetAction() == "opened" { + base, _ := pullCtx.Branches() + if err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base); err != nil { + logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request") + } + } + + if err := h.ProcessPullRequest(ctx, pullCtx, client, config, pr); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") } diff --git a/server/handler/pull_request_review.go b/server/handler/pull_request_review.go index da7e01165..86394fdfd 100644 --- a/server/handler/pull_request_review.go +++ b/server/handler/pull_request_review.go @@ -57,7 +57,11 @@ func (h *PullRequestReview) Handle(ctx context.Context, eventType, deliveryID st } pullCtx := pull.NewGithubContext(client, pr) - if err := h.ProcessPullRequest(ctx, pullCtx, client, pr); err != nil { + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return errors.Wrap(err, "failed to fetch configuration") + } + if err := h.ProcessPullRequest(ctx, pullCtx, client, config, pr); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") } diff --git a/server/handler/push.go b/server/handler/push.go index ff9d2bd65..c29250eca 100644 --- a/server/handler/push.go +++ b/server/handler/push.go @@ -76,8 +76,13 @@ func (h *Push) Handle(ctx context.Context, eventType, deliveryID string, payload pullCtx := pull.NewGithubContext(client, pr) logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return errors.Wrap(err, "failed to fetch configuration") + } + logger.Debug().Msgf("checking status for updated sha %s", baseRef) - if err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, pr, baseRef); err != nil { + if err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, baseRef); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request") } } diff --git a/server/handler/status.go b/server/handler/status.go index 4562e07e1..ceb4a8e6e 100644 --- a/server/handler/status.go +++ b/server/handler/status.go @@ -68,7 +68,11 @@ func (h *Status) Handle(ctx context.Context, eventType, deliveryID string, paylo for _, pr := range prs { pullCtx := pull.NewGithubContext(client, pr) logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() - if err := h.ProcessPullRequest(logger.WithContext(ctx), pullCtx, client, pr); err != nil { + config, err := h.FetchConfig(ctx, client, pr) + if err != nil { + return err + } + if err := h.ProcessPullRequest(logger.WithContext(ctx), pullCtx, client, config, pr); err != nil { logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") } }