refactor: Improve GitHub App installation ID retrieval #2154

chmouel · 2025-07-01T13:32:06Z

Removed listing of all installations.
Fetched the installation directly using the repository URL.
Added fallback to find the organization installation if the repository installation is not found.
Enhanced error handling for token retrieval.

Changes

Submitter Checklist

- Removed listing of all installations. - Fetched the installation directly using the repository URL. - Added fallback to find the organization installation if the repository installation is not found. - Enhanced error handling for token retrieval. - Updated tests to reflect the changes. Signed-off-by: Chmouel Boudjnah <[email protected]>

mathur07 · 2025-07-30T19:28:40Z

This fix will help us resolve this issue: https://issues.redhat.com/browse/KONFLUX-5929

enarha

What's missing in this PR (why is it in draft mode)?

enarha · 2025-07-31T06:21:30Z

pkg/provider/github/app/token.go

-				continue
-			}
-		}
-		exist, err := ip.matchRepos(ctx)


with ip.matchRepos no longer used in this file, we can move it to the test file as it's now used only there.

mathur07 · 2025-07-30T19:11:29Z

pkg/provider/github/app/token.go

+	}
+	owner := pathParts[0]
+	repoName := pathParts[1]
+
 	apiURL := *ip.ghClient.APIURL


Suggested change

apiURL := *ip.ghClient.APIURL

if ip.ghClient.APIURL == nil {

return "", "", 0, fmt.Errorf("github client APIURL is nil")

}

apiURL := *ip.ghClient.APIURL

mathur07 · 2025-07-30T19:23:27Z

pkg/provider/github/app/token.go

 	if enterpriseHost != "" {
-		// NOTE: Hopefully this works even when the GHE URL is on another host than the API URL
 		apiURL = "https://" + enterpriseHost + "/api/v3"


Suggested change

apiURL = "https://" + enterpriseHost + "/api/v3"

apiURL = fmt.Sprintf("https://%s/api/v3", strings.TrimSuffix(enterpriseHost, "/"))

mathur07 · 2025-07-30T20:01:12Z

pkg/provider/github/app/token.go

+	owner := pathParts[0]
+	repoName := pathParts[1]


Should we check them for being empty? Or is it supposed to be always present?
if owner == "" || repoName == "" { return "", "", 0, fmt.Errorf("invalid repository URL: owner or repo name is empty") }

It's not bad to be on the safe side, but tbh that's probably not a concern. It would require there to be // in the path which, while possibly accepted by github, is never going to match the events from Github and therefore isn't valid as a Respository CR.

chmouel · 2025-08-01T10:37:55Z

What's missing in this PR (why is it in draft mode)?

not enough time to work on it for now (and it wasn't prioritized) and generally we don't have enough reviewer in pac so i put thing as draft pr so people don't get stress until i am sure it work...

…retrie

mathur07 · 2025-08-01T14:12:09Z

@chmouel I can give my best effort to review it. @aThorp96 Can help us too in the review?

aThorp96

This is so much cleaner and I think may significantly reduce the number of API calls.

Also, it looks like these API calls are not being included in the pipelines_as_code_git_provider_api_request_count metrics. I don't think that necessarily needs to be clubbed into this PR, but we'll need to fix that as well

aThorp96 · 2025-08-01T14:52:22Z

pkg/provider/github/app/token.go

+	pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/")
+	if len(pathParts) < 2 {
+		return "", "", 0, fmt.Errorf("invalid repository URL path: %s", repoURL.Path)
+	}


Nitpick

Should this be checking for longer paths as well? I know Gitlab supports sub-projects but I don't think Github would support /foo/bar/baz. It technically does resolve for example https://github.com/openshift-pipelines//pipelines-as-code with the two slashes, but since the Repository CR's URL semantically has to match the URL sent in the Github event payload I don't think that // will be valid in the RepositoryCR's URL in the first place.

Suggested change

pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/")

if len(pathParts) < 2 {

return "", "", 0, fmt.Errorf("invalid repository URL path: %s", repoURL.Path)

}

pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/")

if len(pathParts) != 2 {

return "", "", 0, fmt.Errorf("invalid repository URL path: expected '<organization>/<repository>', got: %s", repoURL.Path)

}

aThorp96 · 2025-08-01T14:54:39Z

pkg/provider/github/app/token.go

+	owner := pathParts[0]
+	repoName := pathParts[1]


It's not bad to be on the safe side, but tbh that's probably not a concern. It would require there to be // in the path which, while possibly accepted by github, is never going to match the events from Github and therefore isn't valid as a Respository CR.

aThorp96 · 2025-08-01T15:33:12Z

pkg/provider/github/app/token.go

+	// Directly get the installation for the repository
+	installation, _, err := client.Apps.FindRepositoryInstallation(ctx, owner, repoName)
+	if err != nil {
+		// Fallback to finding organization installation if repository installation is not found
+		installation, _, err = client.Apps.FindOrganizationInstallation(ctx, owner)
 		if err != nil {


Change needed:
The Installation could also be installed for the User as well, so we'll also need to check client.Apps.FindUserInstallation() as well. If only these methods all had the same signature we could just iterate through a list of the methods, alas 😖

aThorp96 · 2025-08-01T15:38:21Z

pkg/provider/github/app/token.go

+	if installation.ID == nil {
+		return "", "", 0, fmt.Errorf("installation ID is nil")
 	}


Nit pick suggestion - in case a user is reading this, a little more verbosity here could help understand what this error means. I don't know when (if ever) the installation ID might be nil but I can only assume that it would indicate a bug in how we use the API or a bug in the github-go library

Suggested change

if installation.ID == nil {

return "", "", 0, fmt.Errorf("installation ID is nil")

}

if installation.ID == nil {

return "", "", 0, fmt.Errorf("Github App installation found but contained no ID. This is likely a bug")

}

aThorp96 · 2025-08-01T15:40:05Z

pkg/provider/github/app/token.go

+	installationID := *installation.ID
+	token, err := ip.ghClient.GetAppToken(ctx, ip.run.Clients.Kube, enterpriseHost, installationID, ip.namespace)
+	if err != nil {
+		logger.Warnf("Could not get a token for installation ID %d: %v", installationID, err)


Should this be an event instead of just a log? Not sure when events are desirable instead of just logs

aThorp96 · 2025-08-01T15:43:05Z

pkg/provider/github/app/token_test.go

+	mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = fmt.Fprintf(w, `{"id": %d}`, wantID)
+	})


Do you mind adding a test case for the fallback(s) as well?

chmouel · 2025-08-04T14:53:39Z

let's wait for #2199 to ship first to make sure we can instrument this

enarha reviewed Jul 31, 2025

View reviewed changes

mathur07 reviewed Jul 31, 2025

View reviewed changes

Merge branch 'main' into refactor-improve-github-app-installation-id-…

2fb6740

…retrie

aThorp96 reviewed Aug 1, 2025

View reviewed changes

	apiURL = "https://" + enterpriseHost + "/api/v3"
	apiURL = fmt.Sprintf("https://%s/api/v3", strings.TrimSuffix(enterpriseHost, "/"))

refactor: Improve GitHub App installation ID retrieval #2154

Are you sure you want to change the base?

refactor: Improve GitHub App installation ID retrieval #2154

Uh oh!

Conversation

chmouel commented Jul 1, 2025

Changes

Submitter Checklist

Uh oh!

mathur07 commented Jul 30, 2025

Uh oh!

enarha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmouel commented Aug 1, 2025

Uh oh!

mathur07 commented Aug 1, 2025

Uh oh!

aThorp96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmouel commented Aug 4, 2025

Uh oh!

Uh oh!