autoupdate: fix: status of previous HEAD commit is evaluated after ba…

…se branch update (#109)

When a Pull-Request was updated with it's base branch by the
autoupdater, the GitHub Check status of the previous HEAD commit was
evaluated.
If the check status for it was negative, the PR was suspended wrongly.
This PR fixes the issue.

The autoupdater now also logs a warning message if a check status for a
different commit then the current HEAD commit returned by the update
operation was retrieved.
This could happen if the branch is updated between the both API
operations or GitHub returned old information (which can happen).
The autoupdater is not able to handle the situation correctly.

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
 }