github.provider.token, cache github.ListRepos

we were calling: ``` repoList, err := github.ListRepos(ctx, gh) ``` each time on each isntallationData when this call can only be done once and be cached. using a struct and refactor the code to use it. Signed-off-by: Chmouel Boudjnah <[email protected]>
openshift-pipelines · Apr 1, 2024 · 3274759 · 3274759
1 parent c703cd8
commit 3274759
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 22 deletions.
diff --git a/pkg/adapter/incoming.go b/pkg/adapter/incoming.go
@@ -106,7 +106,8 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa
 		gh := github.New()
 		gh.Run = l.run
 		ns := info.GetNS(ctx)
-		enterpriseURL, token, installationID, err := app.GetAndUpdateInstallationID(ctx, req, l.run, repo, gh, ns)
+		ip := app.NewInstallation(req, l.run, repo, gh, ns)
+		enterpriseURL, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
 		if err != nil {
 			return false, nil, err
 		}

diff --git a/pkg/cmd/tknpac/info/install.go b/pkg/cmd/tknpac/info/install.go
@@ -64,7 +64,8 @@ func install(ctx context.Context, run *params.Run, ios *cli.IOStreams, apiURL st
 		return err
 	}
 	info := &InstallInfo{run: run, apiURL: apiURL}
-	if jwtToken, err := app.GenerateJWT(ctx, targetNs, info.run); err == nil {
+	ip := app.NewInstallation(nil, run, nil, nil, targetNs)
+	if jwtToken, err := ip.GenerateJWT(ctx); err == nil {
 		info.jwtToken = jwtToken
 		if err := info.get(ctx); err != nil {
 			return err

diff --git a/pkg/provider/github/app/token.go b/pkg/provider/github/app/token.go
@@ -17,24 +17,47 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github"
 )
 
-func GetAndUpdateInstallationID(ctx context.Context, req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, ns string) (string, string, int64, error) {
+type Install struct {
+	request   *http.Request
+	run       *params.Run
+	repo      *v1alpha1.Repository
+	ghClient  *github.Provider
+	namespace string
+
+	repoList []string
+}
+
+func NewInstallation(req *http.Request, run *params.Run, repo *v1alpha1.Repository, gh *github.Provider, namespace string) *Install {
+	if req == nil {
+		req = &http.Request{}
+	}
+	return &Install{
+		request:   req,
+		run:       run,
+		repo:      repo,
+		ghClient:  gh,
+		namespace: namespace,
+	}
+}
+
+func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, string, int64, error) {
 	var (
 		enterpriseHost, token string
 		installationID        int64
 	)
-	jwtToken, err := GenerateJWT(ctx, ns, run)
+	jwtToken, err := ip.GenerateJWT(ctx)
 	if err != nil {
 		return "", "", 0, err
 	}
 
-	installationURL := *gh.APIURL + keys.InstallationURL
-	enterpriseHost = req.Header.Get("X-GitHub-Enterprise-Host")
+	installationURL := *ip.ghClient.APIURL + keys.InstallationURL
+	enterpriseHost = ip.request.Header.Get("X-GitHub-Enterprise-Host")
 	if enterpriseHost != "" {
 		// NOTE: Hopefully this works even when the ghe URL is on another host than the api URL
 		installationURL = "https://" + enterpriseHost + "/api/v3" + keys.InstallationURL
 	}
 
-	res, err := GetReponse(ctx, http.MethodGet, installationURL, jwtToken, run)
+	res, err := GetReponse(ctx, http.MethodGet, installationURL, jwtToken, ip.run)
 	if err != nil {
 		return "", "", 0, err
 	}
@@ -62,12 +85,12 @@ func GetAndUpdateInstallationID(ctx context.Context, req *http.Request, run *par
 			return "", "", 0, fmt.Errorf("installation ID is nil")
 		}
 		if *installationData[i].ID != 0 {
-			token, err = gh.GetAppToken(ctx, run.Clients.Kube, enterpriseHost, *installationData[i].ID, ns)
+			token, err = ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, *installationData[i].ID, ip.namespace)
 			if err != nil {
 				return "", "", 0, err
 			}
 		}
-		exist, err := listRepos(ctx, repo, gh)
+		exist, err := ip.listRepos(ctx)
 		if err != nil {
 			return "", "", 0, err
 		}
@@ -79,14 +102,17 @@ func GetAndUpdateInstallationID(ctx context.Context, req *http.Request, run *par
 	return enterpriseHost, token, installationID, nil
 }
 
-func listRepos(ctx context.Context, repo *v1alpha1.Repository, gh *github.Provider) (bool, error) {
-	repoList, err := github.ListRepos(ctx, gh)
-	if err != nil {
-		return false, err
+func (ip *Install) listRepos(ctx context.Context) (bool, error) {
+	if ip.repoList == nil {
+		var err error
+		ip.repoList, err = github.ListRepos(ctx, ip.ghClient)
+		if err != nil {
+			return false, err
+		}
 	}
-	for i := range repoList {
+	for i := range ip.repoList {
 		// If URL matches with repo spec url then we can break for loop
-		if repoList[i] == repo.Spec.URL {
+		if ip.repoList[i] == ip.repo.Spec.URL {
 			return true, nil
 		}
 	}
@@ -98,11 +124,11 @@ type JWTClaim struct {
 	jwt.RegisteredClaims
 }
 
-func GenerateJWT(ctx context.Context, ns string, run *params.Run) (string, error) {
+func (ip *Install) GenerateJWT(ctx context.Context) (string, error) {
 	// TODO: move this out of here
 	gh := github.New()
-	gh.Run = run
-	applicationID, privateKey, err := gh.GetAppIDAndPrivateKey(ctx, ns, run.Clients.Kube)
+	gh.Run = ip.run
+	applicationID, privateKey, err := gh.GetAppIDAndPrivateKey(ctx, ip.namespace, ip.run.Clients.Kube)
 	if err != nil {
 		return "", err
 	}

diff --git a/pkg/provider/github/app/token_test.go b/pkg/provider/github/app/token_test.go
@@ -145,7 +145,8 @@ func Test_GenerateJWT(t *testing.T) {
 				},
 			}
 
-			token, err := GenerateJWT(ctx, tt.namespace.GetName(), run)
+			ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName())
+			token, err := ip.GenerateJWT(ctx)
 			if tt.wantErr {
 				assert.Assert(t, err != nil)
 				return
@@ -199,7 +200,8 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
 	ctx = info.StoreCurrentControllerName(ctx, "default")
 	ctx = info.StoreNS(ctx, testNamespace.GetName())
 
-	jwtToken, err := GenerateJWT(ctx, testNamespace.GetName(), run)
+	ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName())
+	jwtToken, err := ip.GenerateJWT(ctx)
 	assert.NilError(t, err)
 	req := httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader(""))
 	repo := &v1alpha1.Repository{
@@ -241,7 +243,8 @@ func Test_GetAndUpdateInstallationID(t *testing.T) {
 		w.Header().Set("Accept", "application/vnd.github+json")
 		_, _ = fmt.Fprint(w, `{"total_count": 1,"repositories": [{"id":1,"html_url": "https://matched/by/incoming"},{"id":2,"html_url": "https://anotherrepo/that/would/failit"}]}`)
 	})
-	_, token, installationID, err := GetAndUpdateInstallationID(ctx, req, run, repo, gprovider, testNamespace.GetName())
+	ip = NewInstallation(req, run, repo, gprovider, testNamespace.GetName())
+	_, token, installationID, err := ip.GetAndUpdateInstallationID(ctx)
 	assert.NilError(t, err)
 	assert.Equal(t, installationID, int64(120))
 	assert.Equal(t, *gprovider.Token, wantToken)
@@ -288,7 +291,9 @@ func Test_ListRepos(t *testing.T) {
 
 	ctx, _ := rtesting.SetupFakeContext(t)
 	gprovider := &github.Provider{Client: fakeclient}
-	exist, err := listRepos(ctx, repo, gprovider)
+	ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")),
+		&params.Run{}, repo, gprovider, testNamespace.GetName())
+	exist, err := ip.listRepos(ctx)
 	assert.NilError(t, err)
 	assert.Equal(t, exist, true)
 }