refactor: Improve GitHub App installation ID retrieval #2154
Conversation
|
This fix will help us resolve this issue: https://issues.redhat.com/browse/KONFLUX-5929 |
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... |
There was a problem hiding this comment.
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
pkg/provider/github/app/token.go
Outdated
| pathParts := strings.Split(strings.Trim(repoURL.Path, "/"), "/") | ||
| if len(pathParts) < 2 { | ||
| return "", "", 0, fmt.Errorf("invalid repository URL path: %s", repoURL.Path) | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } |
| // 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 { |
There was a problem hiding this comment.
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 😖
| 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) |
There was a problem hiding this comment.
Should this be an event instead of just a log? Not sure when events are desirable instead of just logs
| mux.HandleFunc(fmt.Sprintf("/orgs/%s/installation", orgName), func(w http.ResponseWriter, _ *http.Request) { | ||
| _, _ = fmt.Fprintf(w, `{"id": %d}`, wantID) | ||
| }) |
There was a problem hiding this comment.
Do you mind adding a test case for the fallback(s) as well?
|
let's wait for #2199 to ship first to make sure we can instrument this |
|
/gemini review |
chmouel
force-pushed
the
refactor-improve-github-app-installation-id-retrie
branch
from
2fb6740 to
b6b6a83
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the efficiency of retrieving the GitHub App installation ID by fetching it directly for the repository or organization, instead of listing all installations. The new logic is much cleaner and more direct. I've included a few suggestions to further enhance the implementation by removing some now-unused code and improving the robustness of the installation discovery mechanism.
- 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. - Eliminated the matchRepos method from Install struct as it was unused refactor: Improve GitHub App installation ID retrieval Signed-off-by: Chmouel Boudjnah <[email protected]>
chmouel
force-pushed
the
refactor-improve-github-app-installation-id-retrie
branch
from
b6b6a83 to
75709dc
Compare
Changes
Submitter Checklist
📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).
♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.
✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).
📖 Document any user-facing features or changes in behavior.
🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.
🎁 If feasible, add an end-to-end test. See README for details.
🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).
If adding a provider feature, fill in the following details:
(update the provider documentation accordingly)