autoupdate: fix: status of previous HEAD commit is evaluated after base branch update

internal/autoupdate/autoupdate.go

@@ -27,7 +27,7 @@ const defPeriodicTriggerInterval = 30 * time.Minute
 // GithubClient defines the methods of a GithubAPI Client that are used by the
 // autoupdate implementation.
 type GithubClient interface {
-	UpdateBranch(ctx context.Context, owner, repo string, pullRequestNumber int) (changed bool, scheduled bool, err error)
+	UpdateBranch(ctx context.Context, owner, repo string, pullRequestNumber int) (*githubclt.UpdateBranchResult, error)
 	CreateIssueComment(ctx context.Context, owner, repo string, issueOrPRNr int, comment string) error
 	ListPullRequests(ctx context.Context, owner, repo, state, sort, sortDirection string) githubclt.PRIterator
 	ReadyForMerge(ctx context.Context, owner, repo string, prNumber int) (*githubclt.ReadyForMergeStatus, error)

internal/autoupdate/autoupdate_test.go

@@ -94,16 +94,14 @@ func mockSuccessfulGithubUpdateBranchCall(clt *mocks.MockGithubClient, expectedP
 	return clt.
 		EXPECT().
 		UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Eq(expectedPRNr)).
-		Return(branchChanged, false, nil)
+		Return(&githubclt.UpdateBranchResult{HeadCommitID: headCommitID, Changed: branchChanged}, nil)
 }
 
 func mockFailedGithubUpdateBranchCall(clt *mocks.MockGithubClient, expectedPRNr int) *gomock.Call {
 	return clt.
 		EXPECT().
 		UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Eq(expectedPRNr)).
-		DoAndReturn(func(context.Context, string, string, int) (bool, bool, error) {
-			return false, false, errors.New("error mocked by mockFailedGithubUpdateBranchCall")
-		})
+		Return(nil, errors.New("error mocked by mockFailedGithubUpdateBranchCall"))
 }
 
 func mockSuccesssfulCreateIssueCommentCall(clt *mocks.MockGithubClient, expectedPRNr int) *gomock.Call {
@@ -718,9 +716,7 @@ func TestSuccessStatusOrCheckEventResumesPRs(t *testing.T) {
 			ghClient.
 				EXPECT().
 				UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Any()).
-				DoAndReturn(func(context.Context, string, string, int) (bool, bool, error) {
-					return false, false, nil
-				}).
+				Return(&githubclt.UpdateBranchResult{HeadCommitID: headCommitID}, nil).
 				MinTimes(3)
 
 			mergeStatusPr1 := mockReadyForMergeStatus(
@@ -905,9 +901,7 @@ func TestFailedStatusEventSuspendsFirstPR(t *testing.T) {
 			ghClient.
 				EXPECT().
 				UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Any()).
-				DoAndReturn(func(context.Context, string, string, int) (bool, bool, error) {
-					return false, false, nil
-				}).
+				Return(&githubclt.UpdateBranchResult{HeadCommitID: headCommitID}, nil).
 				AnyTimes()
 
 			retryer := goordinator.NewRetryer()
@@ -1163,7 +1157,7 @@ func TestInitialSync(t *testing.T) {
 	ghClient.
 		EXPECT().
 		UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Any()).
-		Return(true, false, nil).
+		Return(&githubclt.UpdateBranchResult{Changed: true, HeadCommitID: headCommitID}, nil).
 		AnyTimes()
 
 	prIterNone := mocks.NewMockPRIterator(mockctrl)
@@ -1541,9 +1535,9 @@ func TestBaseBranchUpdatesBlockUntilFinished(t *testing.T) {
 	ghClient.
 		EXPECT().
 		UpdateBranch(gomock.Any(), gomock.Eq(repoOwner), gomock.Eq(repo), gomock.Any()).
-		DoAndReturn(func(context.Context, string, string, int) (bool, bool, error) {
+		DoAndReturn(func(context.Context, string, string, int) (*githubclt.UpdateBranchResult, error) {
 			atomic.AddInt64(&updateBranchCalls, 1)
-			return true, scheduledReturnVal.Load(), nil
+			return &githubclt.UpdateBranchResult{Changed: true, HeadCommitID: headCommitID, Scheduled: scheduledReturnVal.Load()}, nil
 		}).MinTimes(1)
 
 	mockSuccessfulGithubAddLabelQueueHeadCall(ghClient, prNumber).AnyTimes()

internal/autoupdate/drygithubclient.go

@@ -8,6 +8,8 @@ import (
 	"github.com/simplesurance/goordinator/internal/githubclt"
 )
 
+const headCommitID = "32d4ff96ea72412277bbfd22ff1bab3a5263b415"
+
 // DryGithubClient is a github-client that does not do any changes on github.
 // All operations that could cause a change are simulated and always succeed.
 // All all other operations are forwarded to a wrapped GithubClient.
@@ -23,9 +25,9 @@ func NewDryGithubClient(clt GithubClient, logger *zap.Logger) *DryGithubClient {
 	}
 }
 
-func (c *DryGithubClient) UpdateBranch(context.Context, string, string, int) (bool, bool, error) {
+func (c *DryGithubClient) UpdateBranch(context.Context, string, string, int) (*githubclt.UpdateBranchResult, error) {
 	c.logger.Info("simulated updating of github branch, returning is uptodate")
-	return false, false, nil
+	return &githubclt.UpdateBranchResult{HeadCommitID: headCommitID}, nil
 }
 
 func (c *DryGithubClient) ReadyForMerge(context.Context, string, string, int) (*githubclt.ReadyForMergeStatus, error) {
@@ -34,6 +36,7 @@ func (c *DryGithubClient) ReadyForMerge(context.Context, string, string, int) (*
 	return &githubclt.ReadyForMergeStatus{
 		ReviewDecision: githubclt.ReviewDecisionApproved,
 		CIStatus:       githubclt.CIStatusSuccess,
+		Commit:         headCommitID,
 	}, nil
 }