-
Notifications
You must be signed in to change notification settings - Fork 108
feat(#3468): add new GitHub metrics to scorecard module #3472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,12 @@ import { | |
| DefaultGithubCredentialsProvider, | ||
| ScmIntegrations, | ||
| } from '@backstage/integration'; | ||
| import { GithubRepository } from './types'; | ||
| import { | ||
| GithubRepository, | ||
| PullRequestWithReviews, | ||
| WorkflowRun, | ||
| PullRequestCommitStatus, | ||
| } from './types'; | ||
|
|
||
| export class GithubClient { | ||
| private readonly integrations: ScmIntegrations; | ||
|
|
@@ -48,6 +53,31 @@ export class GithubClient { | |
| }); | ||
| } | ||
|
|
||
| private async getRestConfig( | ||
| url: string, | ||
| ): Promise<{ headers: Record<string, string>; apiBaseUrl: string }> { | ||
| const githubIntegration = this.integrations.github.byUrl(url); | ||
| if (!githubIntegration) { | ||
| throw new Error(`Missing GitHub integration for '${url}'`); | ||
| } | ||
|
|
||
| const credentialsProvider = | ||
| DefaultGithubCredentialsProvider.fromIntegrations(this.integrations); | ||
|
|
||
| const { headers } = await credentialsProvider.getCredentials({ | ||
| url, | ||
| }); | ||
|
|
||
| return { | ||
| headers: { | ||
| ...headers, | ||
| Accept: 'application/vnd.github+json', | ||
| } as Record<string, string>, | ||
| apiBaseUrl: | ||
| githubIntegration.config.apiBaseUrl ?? 'https://api.github.com', | ||
| }; | ||
| } | ||
|
|
||
| async getOpenPullRequestsCount( | ||
| url: string, | ||
| repository: GithubRepository, | ||
|
|
@@ -77,4 +107,214 @@ export class GithubClient { | |
|
|
||
| return response.repository.pullRequests.totalCount; | ||
| } | ||
|
|
||
| async getOpenIssuesCount( | ||
| url: string, | ||
| repository: GithubRepository, | ||
| ): Promise<number> { | ||
| const octokit = await this.getOctokitClient(url); | ||
|
|
||
| const query = ` | ||
| query getOpenIssuesCount($owner: String!, $repo: String!) { | ||
| repository(owner: $owner, name: $repo) { | ||
| issues(states: OPEN) { | ||
| totalCount | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const response = await octokit<{ | ||
| repository: { | ||
| issues: { | ||
| totalCount: number; | ||
| }; | ||
| }; | ||
| }>(query, { | ||
| owner: repository.owner, | ||
| repo: repository.repo, | ||
| }); | ||
|
|
||
| return response.repository.issues.totalCount; | ||
| } | ||
|
|
||
| async getSearchCount( | ||
| url: string, | ||
| repository: GithubRepository, | ||
| searchQuery: string, | ||
| ): Promise<number> { | ||
| const octokit = await this.getOctokitClient(url); | ||
|
|
||
| const fullQuery = `repo:${repository.owner}/${repository.repo} ${searchQuery}`; | ||
|
|
||
| const query = ` | ||
| query getSearchCount($q: String!) { | ||
| search(query: $q, type: ISSUE) { | ||
| issueCount | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const response = await octokit<{ | ||
| search: { | ||
| issueCount: number; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] architectural-coherence getWorkflowRuns uses raw fetch() while every other method uses Octokit GraphQL, creating two distinct credential resolution paths. Suggested fix: Use Octokit REST API client or document why raw fetch is necessary. |
||
| }; | ||
| }>(query, { | ||
| q: fullQuery, | ||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] unbounded-loop getWorkflowRuns() paginates through ALL workflow runs with no upper bound. For very active repositories this could cause excessive API calls and memory consumption. Suggested fix: Add a maximum page count or total results limit. |
||
| return response.search.issueCount; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] ssrf repository.owner and repository.repo are interpolated directly into a REST API URL path without encoding. A malicious catalog entity could redirect the request to an unintended API endpoint. Suggested fix: Apply encodeURIComponent() to both repository.owner and repository.repo. |
||
| } | ||
|
|
||
| async getPullRequestsWithReviews( | ||
| url: string, | ||
| repository: GithubRepository, | ||
| since: string, | ||
| ): Promise<PullRequestWithReviews[]> { | ||
| const octokit = await this.getOctokitClient(url); | ||
|
|
||
| const searchQuery = `repo:${repository.owner}/${repository.repo} is:pr updated:>${since}`; | ||
|
|
||
| const query = ` | ||
| query getPRsWithReviews($q: String!) { | ||
| search(query: $q, type: ISSUE, first: 100) { | ||
| nodes { | ||
| ... on PullRequest { | ||
| createdAt | ||
| mergedAt | ||
| reviews(first: 100) { | ||
| nodes { | ||
| createdAt | ||
| state | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const response = await octokit<{ | ||
| search: { | ||
| nodes: PullRequestWithReviews[]; | ||
| }; | ||
| }>(query, { | ||
| q: searchQuery, | ||
| }); | ||
|
|
||
| return response.search.nodes; | ||
| } | ||
|
|
||
| async getWorkflowRuns( | ||
| url: string, | ||
| repository: GithubRepository, | ||
| since: string, | ||
| ): Promise<WorkflowRun[]> { | ||
| const { headers, apiBaseUrl } = await this.getRestConfig(url); | ||
|
|
||
| const allRuns: WorkflowRun[] = []; | ||
| let page = 1; | ||
| const perPage = 100; | ||
| let hasMore = true; | ||
|
|
||
| while (hasMore) { | ||
| const restUrl = `${apiBaseUrl}/repos/${encodeURIComponent( | ||
| repository.owner, | ||
| )}/${encodeURIComponent( | ||
| repository.repo, | ||
| )}/actions/runs?created=>${since}&per_page=${perPage}&page=${page}`; | ||
| const response = await fetch(restUrl, { headers }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error( | ||
| `GitHub API error: ${response.status} ${response.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| const data = (await response.json()) as { | ||
| workflow_runs: WorkflowRun[]; | ||
| total_count: number; | ||
| }; | ||
|
|
||
| allRuns.push(...data.workflow_runs); | ||
|
|
||
| hasMore = | ||
| allRuns.length < data.total_count && | ||
| data.workflow_runs.length >= perPage; | ||
| page++; | ||
| } | ||
|
|
||
| return allRuns; | ||
| } | ||
|
|
||
| async getPullRequestsWithCommitStatuses( | ||
| url: string, | ||
| repository: GithubRepository, | ||
| since: string, | ||
| ): Promise<PullRequestCommitStatus[]> { | ||
| const octokit = await this.getOctokitClient(url); | ||
|
|
||
| const searchQuery = `repo:${repository.owner}/${repository.repo} is:pr created:>${since}`; | ||
|
|
||
| const query = ` | ||
| query getPRsWithStatuses($q: String!) { | ||
| search(query: $q, type: ISSUE, first: 100) { | ||
| nodes { | ||
| ... on PullRequest { | ||
| createdAt | ||
| commits(first: 100) { | ||
| nodes { | ||
| commit { | ||
| committedDate | ||
| statusCheckRollup { | ||
| state | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const response = await octokit<{ | ||
| search: { | ||
| nodes: Array<{ | ||
| createdAt: string; | ||
| commits: { | ||
| nodes: Array<{ | ||
| commit: { | ||
| committedDate: string; | ||
| statusCheckRollup: { | ||
| state: string; | ||
| } | null; | ||
| }; | ||
| }>; | ||
| }; | ||
| }>; | ||
| }; | ||
| }>(query, { | ||
| q: searchQuery, | ||
| }); | ||
|
|
||
| return response.search.nodes.map(pr => { | ||
| // Find the last commit from the first push (committed on or before PR creation) | ||
| const prCreatedAt = new Date(pr.createdAt).getTime(); | ||
| const firstPushCommits = pr.commits.nodes.filter( | ||
| c => new Date(c.commit.committedDate).getTime() <= prCreatedAt + 60000, // 1 minute tolerance | ||
| ); | ||
|
|
||
| const lastFirstPushCommit = | ||
| firstPushCommits.length > 0 | ||
| ? firstPushCommits[firstPushCommits.length - 1] | ||
| : null; | ||
|
|
||
| return { | ||
| createdAt: pr.createdAt, | ||
| firstPushLastCommitState: | ||
| lastFirstPushCommit?.commit.statusCheckRollup?.state ?? null, | ||
| }; | ||
| }); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] data-truncation
getPullRequestsWithReviews() and getPullRequestsWithCommitStatuses() use first:100 without pagination. More than 100 PRs updated in 7 days causes silent truncation.
Suggested fix: Implement cursor-based pagination or document the 100-PR cap.