Support required statuses for Bulldozer to update PRs #307

wrslatz · 2022-05-02T22:41:31Z

Summary

Closes #259

Add required_statuses to Bulldozer update config
- Refactored ShouldUpdatePR to evaluate.go to be alongside ShouldMergePR and reuse some status checks code
- No tests exist anymore in update_test.go since they all covered ShouldUpdatePR and were refactored to evaluate_test.go
Enable update processing when status or check run events are handled
Additional changes
- Improve logging to differentiate between should merge and should update logs
- Adjusted config tests to better test provided and default config values
- Refactor TestShouldUpdatePR to table-based tests so it is easier to understand the test cases

wrslatz · 2022-05-03T14:56:12Z

So it looks like status changes not being fulfilled is working correctly as implemented 🎉

However, status changes themselves are not triggering an update, from the local testing I've done. Looks like I need to update the check run check_run.go) and status update (status.go) handling to consider updates as well as merges like we do for pushes or other pull events.

I'm not sure what implications this might have. Open to any thoughts or feedback.

bluekeyes · 2022-05-09T20:15:47Z

However, status changes themselves are not triggering an update, from the local testing I've done. Looks like I need to update the check run (check_run.go) and status update (status.go) handling to consider updates as well as merges like we do for pushes or other pull events.

Yes, this sounds correct. Because statuses are pretty frequent events, it's important to filter them if we're not already doing so. At minimum, we should exit the handler immediately for non-successful statuses. Offhand, I'm not sure if there are any other optimizations to make here.

wrslatz · 2022-05-11T17:30:41Z

Yes, this sounds correct. Because statuses are pretty frequent events, it's important to filter them if we're not already doing so. At minimum, we should exit the handler immediately for non-successful statuses.

It looks like both already handle this logic

bulldozer/server/handler/status.go

Lines 52 to 55 in c5b5340

    
           if checkState != "success" { 
        
           	logger.Debug().Msgf("Doing nothing since context state for %q was %q", event.GetContext(), event.GetState()) 
        
           	return nil 
        
           }

bulldozer/server/handler/check_run.go

Lines 47 to 50 in c5b5340

    
           if event.GetAction() != "completed" { 
        
           	logger.Debug().Msgf("Doing nothing since check_run action was %q instead of 'completed'", event.GetAction()) 
        
           	return nil 
        
           }

Offhand, I'm not sure if there are any other optimizations to make here.

It looks like both of these handlers act on every open PR when processing

bulldozer/server/handler/status.go

Lines 72 to 82 in c5b5340

    
           for _, pr := range prs { 
        
           	pullCtx := pull.NewGithubContext(client, pr) 
        
           	logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() 
        
           	config, err := h.FetchConfigForPR(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") 
        
           	} 
        
           }

bulldozer/server/handler/check_run.go

Lines 63 to 86 in c5b5340

    
           	for _, pr := range prs { 
        
           		logger := logger.With().Int(githubapp.LogKeyPRNum, pr.GetNumber()).Logger() 
        
           		ctx := logger.WithContext(ctx) 
        
           		// The PR included in the CheckRun response is very slim on information. 
        
           		// It does not contain the owner information or label information we 
        
           		// need to process the pull request. 
        
           		fullPR, _, err := client.PullRequests.Get(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber()) 
        
           		if err != nil { 
        
           			return errors.Wrapf(err, "failed to fetch PR number %q for CheckRun", pr.GetNumber()) 
        
           		} 
        
           		pullCtx := pull.NewGithubContext(client, fullPR) 
        
           		config, err := h.FetchConfigForPR(ctx, client, fullPR) 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           		if err := h.ProcessPullRequest(ctx, pullCtx, client, config, fullPR); err != nil { 
        
           			logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") 
        
           		} 
        
           	} 
        
           	return nil 
        
           }

Whereas we'd want the update logic to only act on the PR to which the check or status update corresponds?

wrslatz · 2022-05-11T17:41:30Z

Actually, I think I'm misunderstanding how that looping to try to merge works. Perhaps we should try updates in the same places. I'll make an attempt at that now

bluekeyes · 2022-05-12T04:46:41Z

Yeah, both checks and statuses are attached to commits, which could be the HEAD of multiple open PRs (but in practice are usually only part of one.) The loops iterate over all of the open PRs associated with the updated commit, meaning you'll want to check if updates are necessary inside the loop, either before or after checking if merges are necessary.

wrslatz · 2022-05-18T14:15:16Z

I've finished most of the code changes here. I'm rewriting the TestShouldUpdatePR to use a table with named props instead of a matrix so it's easier to understand what exactly is being tested.

Do you think we need the full matrix of all combinations of test scenarios? It seems like a lot of tests, and I'm not sure how valuable all of the permutations are vs having tests covering the major scenarios. I can push what I have so far to help with considering, if needed.

bluekeyes

Thanks for implementing this and especially for updating the tests - I really appreciate you leaving those better than you found them!

Overall this looks good, just a minor behavior question and some style suggestions.

bluekeyes · 2022-05-19T21:33:05Z

bulldozer/evaluate.go

@@ -102,34 +102,34 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
 	if mergeConfig.Ignore.Enabled() {
 		ignored, reason, err := IsPRIgnored(ctx, pullCtx, mergeConfig.Ignore)
 		if err != nil {
-			return false, errors.Wrap(err, "failed to determine if pull request is ignored")
+			return false, errors.Wrap(err, "failed to determine if pull request is ignored for merge")


Thanks for clarifying/deduplicating these messages!

bulldozer/evaluate.go

bluekeyes · 2022-05-19T21:54:58Z

bulldozer/evaluate_test.go

+func TestShouldUpdatePR(t *testing.T) {
+	ctx := context.Background()
+	tests := []struct {
+		name            string


Thanks for updating these tests! The new structure is much easier to read and follow.

When I write tests like this, I tend to use a map where the test name is the key. Do you have any thoughts on that style vs. including the name as a property?

I have no preference either way 👍

Refactored to map in 96baa96. I do kind of like how this format looks. Helps compact the cases down a bit with one less line per case.

bluekeyes · 2022-05-19T21:56:03Z

bulldozer/evaluate_test.go

+			expectingUpdate: true,
+		},
+		// Test error handling
+		// TestShouldUpdatePR TODO: add error scenario coverage


If this is a new TODO, it's probably fine to ignore for now, since I don't think we covered the error cases in the old tests either.

Yep, I'm fine leaving it out. I mainly added it to consider implementing error tests in a future update.

Addressed in 75c6e43

bluekeyes · 2022-05-19T22:11:50Z

server/handler/check_run.go

+			var err error
+			didUpdatePR, err = h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base)
+			if err != nil {
+				logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")
+			}


I think it will be a little simpler (and match the pull request handler) to invert this condition: instead of checking !didUpdatePR outside of this branch, check didUpdatePR here and continue.

Suggested change

var err error

didUpdatePR, err = h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base)

if err != nil {

logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")

}

didUpdatePR, err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base)

if err != nil {

logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")

}

if didUpdatePR {

continue

}

Good suggestion, happy to make this change

Addressed for both check_run and status events in 92aad8f

bluekeyes · 2022-05-20T19:51:46Z

Thanks again. I'll try to make a release with this early next week, but feel free to tag me here if that doesn't happen. For now, you can use the snapshot tag of the Docker image if you'd like to test it out.

wrslatz · 2022-05-20T20:21:02Z

Thanks again. I'll try to make a release with this early next week, but feel free to tag me here if that doesn't happen. For now, you can use the snapshot tag of the Docker image if you'd like to test it out.

Thank you!

wrslatz added 7 commits May 2, 2022 10:50

Add required statuses to update config

56f3d36

Move ShouldUpdatePR to evaluate to match ShouldMergePR for reuse

4107c64

Improve config tests for new and default values

3e2fbb5

Implement required statuses for update

b1e3c54

Make logs for should update vs merge more distinct

f6de650

Fix update ignored when required statuses set

335d27e

Document required statuses config for updates

7d9ea83

wrslatz mentioned this pull request May 3, 2022

Option to only update when certain required status are passing #259

Closed

wrslatz added 2 commits May 12, 2022 09:52

Handle updates on check run and status events

3e9e118

Refactor update test cases to table format

579e995

Finish update test refactor, add required statuses tests

f66e987

wrslatz marked this pull request as ready for review May 19, 2022 19:17

More required status test cases

bb557b8

wrslatz marked this pull request as draft May 19, 2022 21:47

bluekeyes reviewed May 19, 2022

View reviewed changes

wrslatz added 4 commits May 19, 2022 18:40

Document required statuses and explicit trigger for updates

0e1a092

Remove TODO in TestShouldUpdatePR for error tests

75c6e43

Simplify handling updates for check and status events

92aad8f

Refactor TestShouldUpdatePR cases to map

96baa96

wrslatz force-pushed the required-statuses-update branch from cc8eaed to 96baa96 Compare May 19, 2022 22:54

wrslatz marked this pull request as ready for review May 19, 2022 22:58

bluekeyes approved these changes May 20, 2022

View reviewed changes

bluekeyes merged commit 1ec9ef5 into palantir:develop May 20, 2022

wrslatz deleted the required-statuses-update branch June 6, 2022 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support required statuses for Bulldozer to update PRs #307

Support required statuses for Bulldozer to update PRs #307

wrslatz commented May 2, 2022 •

edited

Loading

wrslatz commented May 3, 2022 •

edited

Loading

bluekeyes commented May 9, 2022

wrslatz commented May 11, 2022

wrslatz commented May 11, 2022

bluekeyes commented May 12, 2022

wrslatz commented May 18, 2022

bluekeyes left a comment

bluekeyes May 19, 2022

bluekeyes May 19, 2022

wrslatz May 19, 2022 •

edited

Loading

wrslatz May 19, 2022

bluekeyes May 19, 2022

wrslatz May 19, 2022

wrslatz May 19, 2022

bluekeyes May 19, 2022

wrslatz May 19, 2022

wrslatz May 19, 2022

bluekeyes commented May 20, 2022

wrslatz commented May 20, 2022

Support required statuses for Bulldozer to update PRs #307

Support required statuses for Bulldozer to update PRs #307

Conversation

wrslatz commented May 2, 2022 • edited Loading

Summary

wrslatz commented May 3, 2022 • edited Loading

bluekeyes commented May 9, 2022

wrslatz commented May 11, 2022

wrslatz commented May 11, 2022

bluekeyes commented May 12, 2022

wrslatz commented May 18, 2022

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrslatz May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes commented May 20, 2022

wrslatz commented May 20, 2022

wrslatz commented May 2, 2022 •

edited

Loading

wrslatz commented May 3, 2022 •

edited

Loading

wrslatz May 19, 2022 •

edited

Loading