fix ff allocator for checks client (#258)

runatlantis · Jun 2, 2022 · 96f4564 · 96f4564
1 parent 81c96ea
commit 96f4564
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 62 deletions.
diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go
@@ -1328,7 +1328,7 @@ func (t *testGithubClient) GetRepoStatuses(repo models.Repo, pull models.PullReq
 	return []*github.RepoStatus{}, nil
 
 }
-func (t *testGithubClient) GetRepoChecks(repo models.Repo, pull models.PullRequest) ([]*github.CheckRun, error) {
+func (t *testGithubClient) GetRepoChecks(repo models.Repo, commitSHA string) ([]*github.CheckRun, error) {
 	return []*github.CheckRun{}, nil
 
 }

diff --git a/server/events/vcs/instrumented_client.go b/server/events/vcs/instrumented_client.go
@@ -50,7 +50,7 @@ type IGithubClient interface {
 
 	GetContents(owner, repo, branch, path string) ([]byte, error)
 	GetRepoStatuses(repo models.Repo, pull models.PullRequest) ([]*github.RepoStatus, error)
-	GetRepoChecks(repo models.Repo, pull models.PullRequest) ([]*github.CheckRun, error)
+	GetRepoChecks(repo models.Repo, commitSHA string) ([]*github.CheckRun, error)
 }
 
 // InstrumentedGithubClient should delegate to the underlying InstrumentedClient for vcs provider-agnostic
@@ -118,7 +118,7 @@ func (c *InstrumentedGithubClient) GetPullRequestFromName(repoName string, repoO
 	return pull, err
 }
 
-func (c *InstrumentedGithubClient) GetRepoChecks(repo models.Repo, pull models.PullRequest) ([]*github.CheckRun, error) {
+func (c *InstrumentedGithubClient) GetRepoChecks(repo models.Repo, commitSHA string) ([]*github.CheckRun, error) {
 	scope := c.StatsScope.SubScope("get_repo_checks")
 
 	executionTime := scope.Timer(metrics.ExecutionTimeMetric).Start()
@@ -127,7 +127,7 @@ func (c *InstrumentedGithubClient) GetRepoChecks(repo models.Repo, pull models.P
 	executionSuccess := scope.Counter(metrics.ExecutionSuccessMetric)
 	executionError := scope.Counter(metrics.ExecutionErrorMetric)
 
-	statuses, err := c.GhClient.GetRepoChecks(repo, pull.HeadCommit)
+	statuses, err := c.GhClient.GetRepoChecks(repo, commitSHA)
 
 	if err != nil {
 		executionError.Inc(1)
@@ -137,7 +137,7 @@ func (c *InstrumentedGithubClient) GetRepoChecks(repo models.Repo, pull models.P
 	executionSuccess.Inc(1)
 
 	//TODO: thread context and use related logging methods.
-	c.Logger.Info("fetched vcs repo checks", fields.PullRequest(pull))
+	c.Logger.Info("fetched vcs repo checks", map[string]interface{}{"commitSHA": commitSHA})
 
 	return statuses, err
 }

diff --git a/server/events/vcs/lyft/pull_status_fetcher.go b/server/events/vcs/lyft/pull_status_fetcher.go
@@ -14,7 +14,7 @@ const LockValue = "lock"
 type PullClient interface {
 	GetPullRequest(repo models.Repo, pullNum int) (*github.PullRequest, error)
 	GetRepoStatuses(repo models.Repo, pull models.PullRequest) ([]*github.RepoStatus, error)
-	GetRepoChecks(repo models.Repo, pull models.PullRequest) ([]*github.CheckRun, error)
+	GetRepoChecks(repo models.Repo, commitSHA string) ([]*github.CheckRun, error)
 	PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error)
 }
 
@@ -48,7 +48,7 @@ func (s *SQBasedPullStatusFetcher) FetchPullStatus(repo models.Repo, pull models
 		return pullStatus, errors.Wrap(err, "fetching repo statuses")
 	}
 
-	checks, err := s.client.GetRepoChecks(repo, pull)
+	checks, err := s.client.GetRepoChecks(repo, pull.HeadCommit)
 	if err != nil {
 		return pullStatus, errors.Wrap(err, "fetching repo checks")
 	}

diff --git a/server/events/vcs/mocks/matchers/ptr_to_github_pullrequest.go b/server/events/vcs/mocks/matchers/ptr_to_github_pullrequest.go
diff --git a/server/events/vcs/mocks/matchers/slice_of_ptr_to_github_checkrun.go b/server/events/vcs/mocks/matchers/slice_of_ptr_to_github_checkrun.go
diff --git a/server/events/vcs/mocks/matchers/slice_of_ptr_to_github_repostatus.go b/server/events/vcs/mocks/matchers/slice_of_ptr_to_github_repostatus.go
diff --git a/server/events/vcs/mocks/mock_IGithub_client.go b/server/events/vcs/mocks/mock_IGithub_client.go
diff --git a/server/server.go b/server/server.go
@@ -175,6 +175,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 	var bitbucketCloudClient *bitbucketcloud.Client
 	var bitbucketServerClient *bitbucketserver.Client
 	var azuredevopsClient *vcs.AzureDevopsClient
+	var featureAllocator feature.Allocator
 
 	mergeabilityChecker := vcs.NewLyftPullMergeabilityChecker(userConfig.VCSStatusName)
 
@@ -204,18 +205,6 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		return nil, errors.Wrapf(err, "instantiating metrics scope")
 	}
 
-	featureAllocator, err := feature.NewGHSourcedAllocator(
-		feature.RepoConfig{
-			Owner:  userConfig.FFOwner,
-			Repo:   userConfig.FFRepo,
-			Branch: userConfig.FFBranch,
-			Path:   userConfig.FFPath,
-		}, githubClient, ctxLogger)
-
-	if err != nil {
-		return nil, errors.Wrap(err, "initializing feature allocator")
-	}
-
 	if userConfig.GithubUser != "" || userConfig.GithubAppID != 0 {
 		supportedVCSHosts = append(supportedVCSHosts, models.Github)
 		if userConfig.GithubUser != "" {
@@ -252,6 +241,18 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 			return nil, err
 		}
 
+		featureAllocator, err = feature.NewGHSourcedAllocator(
+			feature.RepoConfig{
+				Owner:  userConfig.FFOwner,
+				Repo:   userConfig.FFRepo,
+				Branch: userConfig.FFBranch,
+				Path:   userConfig.FFPath,
+			}, rawGithubClient, ctxLogger)
+
+		if err != nil {
+			return nil, errors.Wrap(err, "initializing feature allocator")
+		}
+
 		// [WENGINES-4643] TODO: Remove this wrapped client once github checks is stable
 		checksWrapperGhClient := &lyft_checks.ChecksClientWrapper{
 			FeatureAllocator: featureAllocator,