Skip to content

Commit ab9cd51

Browse files
itaybreclaude
andcommitted
fix(github): Filter artifacts by name when fetching revision artifact
Listing all artifacts of a repository via the GitHub API is slow and fails with HTTP 500 on repositories with many artifacts, which breaks releases for projects using the legacy artifact lookup where the artifact name equals the revision SHA. Pass the revision as the name filter so GitHub only returns matching artifacts. This avoids the failing unfiltered listing and removes the need to page through unrelated artifacts. Fixes GH-879 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 960a1c7 commit ab9cd51

2 files changed

Lines changed: 77 additions & 1 deletion

File tree

‎src/artifact_providers/__tests__/github.test.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,31 @@ describe('GitHub Artifact Provider', () => {
410410
expect(sleep).toBeCalledTimes(2);
411411
});
412412

413+
test('it should filter artifacts by revision name on every retry', async () => {
414+
mockClient.actions.listArtifactsForRepo.mockResolvedValue({
415+
status: 200,
416+
data: {
417+
total_count: 0,
418+
artifacts: [],
419+
},
420+
});
421+
422+
await expect(
423+
githubArtifactProvider.testGetRevisionArtifact(
424+
'1b843f2cbb20fdda99ef749e29e75e43e6e43b38',
425+
),
426+
).rejects.toThrow();
427+
428+
expect(mockClient.actions.listArtifactsForRepo).toBeCalledTimes(3);
429+
for (const call of mockClient.actions.listArtifactsForRepo.mock.calls) {
430+
expect(call[0]).toMatchObject({
431+
owner: 'getsentry',
432+
repo: 'craft',
433+
name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38',
434+
});
435+
}
436+
});
437+
413438
test('it should throw when no artifacts with the name can be found', async () => {
414439
mockClient.actions.listArtifactsForRepo.mockResolvedValue({
415440
status: 200,
@@ -457,6 +482,53 @@ describe('GitHub Artifact Provider', () => {
457482
});
458483

459484
describe('searchForRevisionArtifact', () => {
485+
test('it should filter artifacts by revision name server-side', async () => {
486+
// Listing all artifacts of a repository can fail with HTTP 500 on
487+
// repositories with many artifacts, so the API must be asked to filter
488+
// by name (see https://github.com/getsentry/craft/issues/879).
489+
mockClient.actions.listArtifactsForRepo.mockResolvedValueOnce({
490+
status: 200,
491+
data: {
492+
total_count: 1,
493+
artifacts: [
494+
{
495+
id: 60233710,
496+
node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==',
497+
name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38',
498+
size_in_bytes: 6511029,
499+
url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710',
500+
archive_download_url:
501+
'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip',
502+
expired: false,
503+
created_at: '2021-05-12T21:50:35Z',
504+
updated_at: '2021-05-12T21:50:38Z',
505+
expires_at: '2021-08-10T21:50:31Z',
506+
},
507+
],
508+
},
509+
});
510+
511+
const getRevisionDateCallback = vi.fn();
512+
513+
const artifact =
514+
await githubArtifactProvider.testSearchForRevisionArtifact(
515+
'1b843f2cbb20fdda99ef749e29e75e43e6e43b38',
516+
lazyRequest<string>(getRevisionDateCallback),
517+
);
518+
519+
expect(artifact?.id).toBe(60233710);
520+
expect(mockClient.actions.listArtifactsForRepo).toBeCalledTimes(1);
521+
expect(mockClient.actions.listArtifactsForRepo).toBeCalledWith({
522+
owner: 'getsentry',
523+
repo: 'craft',
524+
name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38',
525+
per_page: 100,
526+
page: 0,
527+
});
528+
// A single page of filtered results should not require the commit date.
529+
expect(getRevisionDateCallback).not.toBeCalled();
530+
});
531+
460532
test('it should get the artifact from second page', async () => {
461533
mockClient.actions.listArtifactsForRepo
462534
.mockResolvedValueOnce({

‎src/artifact_providers/github.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,14 @@ export class GitHubArtifactProvider extends BaseArtifactProvider {
136136

137137
let checkNextPage = true;
138138
for (let page = 0; checkNextPage; page++) {
139-
// https://docs.github.com/en/free-pro-team@latest/rest/reference/actions#artifacts
139+
// https://docs.github.com/en/rest/actions/artifacts#list-artifacts-for-a-repository
140+
// Filter by name server-side: listing all artifacts of a repository
141+
// with a large number of artifacts is slow and can fail with HTTP 500
142+
// (see https://github.com/getsentry/craft/issues/879).
140143
const artifactResponse = await this.github.actions.listArtifactsForRepo({
141144
owner: owner,
142145
repo: repo,
146+
name: revision,
143147
per_page,
144148
page,
145149
});

0 commit comments

Comments
 (0)