From 454d17c9902cc2e21dc46be8594cc4830e1e66fd Mon Sep 17 00:00:00 2001 From: secustor Date: Tue, 12 Dec 2023 11:45:57 +0100 Subject: [PATCH 1/8] refactor(backend-common/GithubUrlReader): only call fetch inside fetchResponse Signed-off-by: secustor --- .changeset/eight-rice-cross.md | 5 ++ .../src/reading/GithubUrlReader.ts | 55 ++++++++----------- 2 files changed, 28 insertions(+), 32 deletions(-) create mode 100644 .changeset/eight-rice-cross.md diff --git a/.changeset/eight-rice-cross.md b/.changeset/eight-rice-cross.md new file mode 100644 index 0000000000..e40f731a4e --- /dev/null +++ b/.changeset/eight-rice-cross.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Do not call fetch directly but rather use fetchResponse facility diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 764431386d..175815cabf 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -107,7 +107,7 @@ export class GithubUrlReader implements UrlReader { let response: Response; try { - response = await fetch(ghUrl, { + response = await this.fetchResponse(ghUrl, { headers: { ...credentials?.headers, ...(options?.etag && { 'If-None-Match': options.etag }), @@ -125,38 +125,13 @@ export class GithubUrlReader implements UrlReader { signal: options?.signal as any, }); } catch (e) { - throw new Error(`Unable to read ${url}, ${e}`); + throw e; } - if (response.status === 304) { - throw new NotModifiedError(); - } - - if (response.ok) { - return ReadUrlResponseFactory.fromNodeJSReadable(response.body, { - etag: response.headers.get('ETag') ?? undefined, - lastModifiedAt: parseLastModified( - response.headers.get('Last-Modified'), - ), - }); - } - - let message = `${url} could not be read as ${ghUrl}, ${response.status} ${response.statusText}`; - if (response.status === 404) { - throw new NotFoundError(message); - } - - // GitHub returns a 403 response with a couple of headers indicating rate - // limit status. See more in the GitHub docs: - // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting - if ( - response.status === 403 && - response.headers.get('X-RateLimit-Remaining') === '0' - ) { - message += ' (rate limit exceeded)'; - } - - throw new Error(message); + return ReadUrlResponseFactory.fromNodeJSReadable(response.body, { + etag: response.headers.get('ETag') ?? undefined, + lastModifiedAt: parseLastModified(response.headers.get('Last-Modified')), + }); } async readTree( @@ -350,10 +325,26 @@ export class GithubUrlReader implements UrlReader { const response = await fetch(urlAsString, init); if (!response.ok) { - const message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`; + let message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`; + + if (response.status === 304) { + throw new NotModifiedError(); + } + if (response.status === 404) { throw new NotFoundError(message); } + + // GitHub returns a 403 response with a couple of headers indicating rate + // limit status. See more in the GitHub docs: + // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting + if ( + response.status === 403 && + response.headers.get('X-RateLimit-Remaining') === '0' + ) { + message += ' (rate limit exceeded)'; + } + throw new Error(message); } From 88fcc3b5e56d839b32492ddee45c40fc010514bf Mon Sep 17 00:00:00 2001 From: secustor Date: Tue, 12 Dec 2023 11:53:04 +0100 Subject: [PATCH 2/8] docs(backend-common/GithubUrlReader): put fetchResponse in code block Signed-off-by: secustor --- .changeset/eight-rice-cross.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/eight-rice-cross.md b/.changeset/eight-rice-cross.md index e40f731a4e..1628276817 100644 --- a/.changeset/eight-rice-cross.md +++ b/.changeset/eight-rice-cross.md @@ -2,4 +2,4 @@ '@backstage/backend-common': patch --- -Do not call fetch directly but rather use fetchResponse facility +Do not call fetch directly but rather use `fetchResponse` facility From 42b4db71ded631e9a286c6b19c6fe16fc211f184 Mon Sep 17 00:00:00 2001 From: secustor Date: Tue, 12 Dec 2023 14:37:11 +0100 Subject: [PATCH 3/8] make fetchResponse protected to allow overwriting Signed-off-by: secustor --- packages/backend-common/src/reading/GithubUrlReader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 175815cabf..fed6fd9430 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -317,7 +317,7 @@ export class GithubUrlReader implements UrlReader { return repo.default_branch; } - private async fetchResponse( + protected async fetchResponse( url: string | URL, init: RequestInit, ): Promise { From f4d77420496ff7bb9092a56509bcd643208beac4 Mon Sep 17 00:00:00 2001 From: secustor Date: Thu, 14 Dec 2023 15:50:16 +0100 Subject: [PATCH 4/8] remove unnecessary try catch block Signed-off-by: secustor --- .../src/reading/GithubUrlReader.ts | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index fed6fd9430..6fd24afc08 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -105,28 +105,23 @@ export class GithubUrlReader implements UrlReader { credentials, ); - let response: Response; - try { - response = await this.fetchResponse(ghUrl, { - headers: { - ...credentials?.headers, - ...(options?.etag && { 'If-None-Match': options.etag }), - ...(options?.lastModifiedAfter && { - 'If-Modified-Since': options.lastModifiedAfter.toUTCString(), - }), - Accept: 'application/vnd.github.v3.raw', - }, - // TODO(freben): The signal cast is there because pre-3.x versions of - // node-fetch have a very slightly deviating AbortSignal type signature. - // The difference does not affect us in practice however. The cast can - // be removed after we support ESM for CLI dependencies and migrate to - // version 3 of node-fetch. - // https://github.com/backstage/backstage/issues/8242 - signal: options?.signal as any, - }); - } catch (e) { - throw e; - } + const response = await this.fetchResponse(ghUrl, { + headers: { + ...credentials?.headers, + ...(options?.etag && { 'If-None-Match': options.etag }), + ...(options?.lastModifiedAfter && { + 'If-Modified-Since': options.lastModifiedAfter.toUTCString(), + }), + Accept: 'application/vnd.github.v3.raw', + }, + // TODO(freben): The signal cast is there because pre-3.x versions of + // node-fetch have a very slightly deviating AbortSignal type signature. + // The difference does not affect us in practice however. The cast can + // be removed after we support ESM for CLI dependencies and migrate to + // version 3 of node-fetch. + // https://github.com/backstage/backstage/issues/8242 + signal: options?.signal as any, + }); return ReadUrlResponseFactory.fromNodeJSReadable(response.body, { etag: response.headers.get('ETag') ?? undefined, From 7e7319bcb00d4c6f9ce4ffffd2234727ad1e17c7 Mon Sep 17 00:00:00 2001 From: Daniel Laird Date: Fri, 12 Jan 2024 15:31:56 +0000 Subject: [PATCH 5/8] Ensure teamReviewer list contains unique team names before sending to API Signed-off-by: Daniel Laird --- .../src/actions/githubPullRequest.test.ts | 4 ++-- .../src/actions/githubPullRequest.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts index 23ac4bda45..c5b1038a01 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.test.ts @@ -377,7 +377,7 @@ describe('createPublishGithubPullRequestAction', () => { branchName: 'new-app', description: 'This PR is really good', reviewers: ['foobar'], - teamReviewers: ['team-foo'], + teamReviewers: ['team-foo', 'team-foo', 'team-bar'], }; mockDir.setContent({ [workspacePath]: {} }); @@ -401,7 +401,7 @@ describe('createPublishGithubPullRequestAction', () => { repo: 'myrepo', pull_number: 123, reviewers: ['foobar'], - team_reviewers: ['team-foo'], + team_reviewers: ['team-foo', 'team-bar'], }); }); diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts index 0763d4a474..2d8dc7ce8d 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubPullRequest.ts @@ -372,7 +372,7 @@ export const createPublishGithubPullRequestAction = ( repo: pr.repo, pull_number: pr.number, reviewers, - team_reviewers: teamReviewers, + team_reviewers: teamReviewers ? [...new Set(teamReviewers)] : undefined, }); const addedUsers = result.data.requested_reviewers?.join(', ') ?? ''; const addedTeams = result.data.requested_teams?.join(', ') ?? ''; From 547030034df4d263a28fe423b3e7a9db2ddf9b38 Mon Sep 17 00:00:00 2001 From: Daniel Laird Date: Fri, 12 Jan 2024 15:33:29 +0000 Subject: [PATCH 6/8] Add Changeset Signed-off-by: Daniel Laird --- .changeset/fair-spies-rescue.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fair-spies-rescue.md diff --git a/.changeset/fair-spies-rescue.md b/.changeset/fair-spies-rescue.md new file mode 100644 index 0000000000..2b49d4b969 --- /dev/null +++ b/.changeset/fair-spies-rescue.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-github': minor +--- + +Ensure `teamReviewers` list is unique before calling API From b6b11677582ffe03c5df05b8e9130d2e2c62d96b Mon Sep 17 00:00:00 2001 From: secustor Date: Fri, 12 Jan 2024 20:05:19 +0100 Subject: [PATCH 7/8] chore: revert exposing of fetchResponse Signed-off-by: secustor --- packages/backend-common/src/reading/GithubUrlReader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 6fd24afc08..67f6a975c1 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -312,7 +312,7 @@ export class GithubUrlReader implements UrlReader { return repo.default_branch; } - protected async fetchResponse( + private async fetchResponse( url: string | URL, init: RequestInit, ): Promise { From 9373b4c88ef97306d7f699d21428e5b7f0942f8f Mon Sep 17 00:00:00 2001 From: Daniel Laird Date: Mon, 15 Jan 2024 14:21:23 +0000 Subject: [PATCH 8/8] Resolve PR feedback Signed-off-by: Daniel Laird --- .changeset/fair-spies-rescue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/fair-spies-rescue.md b/.changeset/fair-spies-rescue.md index 2b49d4b969..d7f1ea5275 100644 --- a/.changeset/fair-spies-rescue.md +++ b/.changeset/fair-spies-rescue.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-scaffolder-backend-module-github': minor +'@backstage/plugin-scaffolder-backend-module-github': patch --- Ensure `teamReviewers` list is unique before calling API