-
Notifications
You must be signed in to change notification settings - Fork 361
copilot: add missing pagination to fetchOrganizationTeams #3170
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
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a04492e
(fix) Add missing pagination to fetchOrganizationTeams
christianherweg0807 cb49af2
Merge branch 'main' into main
christianherweg0807 a10aa04
[cleanup] simplify codebase for paging
christianherweg0807 60c8237
[fix] fix tsc error & use logger service
christianherweg0807 2d04a16
[changeset] Add the missing changeset
christianherweg0807 b24b737
Merge branch 'main' into main
christianherweg0807 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@backstage-community/plugin-copilot-backend': patch | ||
--- | ||
|
||
Add pagination to the CoPilot backend plugins GithubClient. Fixed a bug not iterating over all teams in github. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,12 @@ | |
|
||
import { ResponseError } from '@backstage/errors'; | ||
import { Config } from '@backstage/config'; | ||
import { LoggerService } from '@backstage/backend-plugin-api'; | ||
import { | ||
CopilotMetrics, | ||
TeamInfo, | ||
} from '@backstage-community/plugin-copilot-common'; | ||
import fetch from 'node-fetch'; | ||
import fetch, { Response as NodeFetchResponse } from 'node-fetch'; | ||
import { | ||
CopilotConfig, | ||
CopilotCredentials, | ||
|
@@ -43,14 +44,25 @@ interface GithubApi { | |
} | ||
|
||
export class GithubClient implements GithubApi { | ||
private readonly logger: LoggerService; | ||
|
||
constructor( | ||
private readonly copilotConfig: CopilotConfig, | ||
private readonly config: Config, | ||
) {} | ||
logger?: LoggerService, | ||
) { | ||
this.logger = logger || { | ||
debug: () => {}, | ||
info: () => {}, | ||
warn: () => {}, | ||
error: () => {}, | ||
child: () => this.logger, | ||
}; | ||
} | ||
|
||
static async fromConfig(config: Config) { | ||
static async fromConfig(config: Config, logger?: LoggerService) { | ||
const info = getCopilotConfig(config); | ||
return new GithubClient(info, config); | ||
return new GithubClient(info, config, logger); | ||
} | ||
|
||
private async getCredentials(): Promise<CopilotCredentials> { | ||
|
@@ -87,8 +99,38 @@ export class GithubClient implements GithubApi { | |
} | ||
|
||
async fetchOrganizationTeams(): Promise<TeamInfo[]> { | ||
const path = `/orgs/${this.copilotConfig.organization}/teams`; | ||
return this.get(path); | ||
const perPage = 100; | ||
let page = 1; | ||
let allTeams: TeamInfo[] = []; | ||
let hasNextPage = true; | ||
|
||
while (hasNextPage) { | ||
const path = `/orgs/${this.copilotConfig.organization}/teams?per_page=${perPage}&page=${page}`; | ||
|
||
// Use the raw response method to access headers | ||
const response = await this.getRaw(path); | ||
const teams = (await response.json()) as TeamInfo[]; | ||
|
||
if (Array.isArray(teams)) { | ||
allTeams = [...allTeams, ...teams]; | ||
} else { | ||
throw new Error( | ||
`Invalid response format: expected array but got ${typeof teams}`, | ||
); | ||
} | ||
|
||
// Check for pagination using GitHub's Link header | ||
const linkHeader = response.headers.get('link'); | ||
hasNextPage = Boolean(linkHeader && linkHeader.includes('rel="next"')); | ||
page++; | ||
|
||
// Break if we got fewer results than requested (last page) | ||
if (teams.length < perPage) { | ||
hasNextPage = false; | ||
} | ||
} | ||
this.logger.info(`Fetched ${allTeams.length} teams`); | ||
return allTeams; | ||
} | ||
|
||
private async get<T>(path: string): Promise<T> { | ||
|
@@ -111,4 +153,26 @@ export class GithubClient implements GithubApi { | |
|
||
return response.json() as Promise<T>; | ||
} | ||
|
||
// Add this new private method to handle raw responses | ||
private async getRaw(path: string): Promise<NodeFetchResponse> { | ||
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. This can possibly be simplified in order to reduce duplicated code. Maybe the get() can use getRaw() since the only thing that differs is the return. |
||
const credentials = await this.getCredentials(); | ||
const headers = path.startsWith('/enterprises') | ||
? credentials.enterprise?.headers | ||
: credentials.organization?.headers; | ||
|
||
const response = await fetch(`${this.copilotConfig.apiBaseUrl}${path}`, { | ||
headers: { | ||
...headers, | ||
Accept: 'application/vnd.github+json', | ||
'X-GitHub-Api-Version': '2022-11-28', | ||
}, | ||
}); | ||
|
||
if (!response.ok) { | ||
throw await ResponseError.fromResponse(response); | ||
} | ||
|
||
return response; | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think it would be better to make the logger mandatory to provide.
You can provide it as a dependency here: https://github.com/backstage/community-plugins/blob/main/workspaces/copilot/plugins/copilot-backend/src/service/router.ts#L126
And it is availible here to grab: https://github.com/backstage/community-plugins/blob/main/workspaces/copilot/plugins/copilot-backend/src/service/router.ts#L122