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

@@ -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.
 

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

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")
 		}
 	}

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")
 	}
 

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")
 	}
 

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")
 	}
 

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")
 		}
 	}

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")
 		}
 	}