Fetch configuration once when updating PRs (#277)

Previously, we fetched the same configuration file for every PR that
might be updated. This was a lot of redundant requests, especially if
the repository uses remote configuration.

Author: bluekeyes

server/handler/base.go

@@ -32,10 +32,17 @@ type Base struct {
 	PushRestrictionUserToken string
 }
 
-func (b *Base) FetchConfig(ctx context.Context, client *github.Client, pr *github.PullRequest) (*bulldozer.Config, error) {
+func (b *Base) FetchConfigForPR(ctx context.Context, client *github.Client, pr *github.PullRequest) (*bulldozer.Config, error) {
+	owner := pr.GetBase().GetRepo().GetOwner().GetLogin()
+	repo := pr.GetBase().GetRepo().GetName()
+	ref := pr.GetBase().GetRef()
+	return b.FetchConfig(ctx, client, owner, repo, ref)
+}
+
+func (b *Base) FetchConfig(ctx context.Context, client *github.Client, owner, repo, ref string) (*bulldozer.Config, error) {
 	logger := zerolog.Ctx(ctx)
 
-	fc := b.ConfigFetcher.ConfigForPR(ctx, client, pr)
+	fc := b.ConfigFetcher.Config(ctx, client, owner, repo, ref)
 	switch {
 	case fc.LoadError != nil:
 		return nil, errors.Wrapf(fc.LoadError, "failed to load configuration: %s: %s", fc.Source, fc.Path)

server/handler/check_run.go

@@ -73,7 +73,7 @@ func (h *CheckRun) Handle(ctx context.Context, eventType, deliveryID string, pay
 		}
 		pullCtx := pull.NewGithubContext(client, fullPR)
 
-		config, err := h.FetchConfig(ctx, client, fullPR)
+		config, err := h.FetchConfigForPR(ctx, client, fullPR)
 		if err != nil {
 			return err
 		}

server/handler/fetcher.go

@@ -44,13 +44,9 @@ func NewConfigFetcher(loader *appconfig.Loader, defaultConfig *bulldozer.Config)
 	}
 }
 
-func (cf *ConfigFetcher) ConfigForPR(ctx context.Context, client *github.Client, pr *github.PullRequest) FetchedConfig {
+func (cf *ConfigFetcher) Config(ctx context.Context, client *github.Client, owner, repo, ref string) FetchedConfig {
 	logger := zerolog.Ctx(ctx)
 
-	owner := pr.GetBase().GetRepo().GetOwner().GetLogin()
-	repo := pr.GetBase().GetRepo().GetName()
-	ref := pr.GetBase().GetRef()
-
 	c, err := cf.loader.LoadConfig(ctx, client, owner, repo, ref)
 	fc := FetchedConfig{
 		Source: c.Source,

server/handler/issue_comment.go

@@ -56,7 +56,7 @@ func (h *IssueComment) Handle(ctx context.Context, eventType, deliveryID string,
 	}
 	pullCtx := pull.NewGithubContext(client, pr)
 
-	config, err := h.FetchConfig(ctx, client, pr)
+	config, err := h.FetchConfigForPR(ctx, client, pr)
 	if err != nil {
 		return err
 	}

server/handler/pull_request.go

@@ -63,7 +63,7 @@ func (h *PullRequest) Handle(ctx context.Context, eventType, deliveryID string,
 	}
 	pullCtx := pull.NewGithubContext(client, pr)
 
-	config, err := h.FetchConfig(ctx, client, pr)
+	config, err := h.FetchConfigForPR(ctx, client, pr)
 	if err != nil {
 		return err
 	}

server/handler/pull_request_review.go

@@ -56,7 +56,7 @@ func (h *PullRequestReview) Handle(ctx context.Context, eventType, deliveryID st
 	}
 	pullCtx := pull.NewGithubContext(client, pr)
 
-	config, err := h.FetchConfig(ctx, client, pr)
+	config, err := h.FetchConfigForPR(ctx, client, pr)
 	if err != nil {
 		return errors.Wrap(err, "failed to fetch configuration")
 	}

server/handler/push.go

@@ -53,6 +53,7 @@ func (h *Push) Handle(ctx context.Context, eventType, deliveryID string, payload
 	}
 
 	ctx, logger := githubapp.PrepareRepoContext(ctx, installationID, ghRepo)
+	logger.Debug().Msgf("Received push event with base ref %s", baseRef)
 
 	client, err := h.ClientCreator.NewInstallationClient(installationID)
 	if err != nil {
@@ -63,24 +64,22 @@ func (h *Push) Handle(ctx context.Context, eventType, deliveryID string, payload
 	if err != nil {
 		return errors.Wrap(err, "failed to determine open pull requests matching the push change")
 	}
-
-	logger.Debug().Msgf("received push event with base ref %s", baseRef)
-
 	if len(prs) == 0 {
-		logger.Debug().Msg("Doing nothing since push event affects no open pull requests")
+		logger.Debug().Msgf("Doing nothing since push to %s affects no open pull requests", baseRef)
 		return nil
 	}
 
+	// Fetch configuration once, since we know all PRs target the same ref
+	config, err := h.FetchConfig(ctx, client, owner, repoName, baseRef)
+	if err != nil {
+		return err
+	}
+
 	for _, pr := range prs {
-		pullCtx := pull.NewGithubContext(client, pr)
 		logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger()
+		logger.Debug().Msgf("Considering pull request for update")
 
-		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)
+		pullCtx := pull.NewGithubContext(client, pr)
 		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

@@ -67,7 +67,7 @@ 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()
-		config, err := h.FetchConfig(ctx, client, pr)
+		config, err := h.FetchConfigForPR(ctx, client, pr)
 		if err != nil {
 			return err
 		}