From 6c70919f1ad6412d1d7f8bba5e20a184428c965c Mon Sep 17 00:00:00 2001 From: Christopher Kruse Date: Mon, 30 Jan 2023 10:58:49 -0800 Subject: [PATCH 1/3] fix: better error around github teams not found Adds a literate error around the `createGithubRepoWithCollaboratorsAndTopics` function in order to provide that error in the scaffolder logs. Prior to this, the only thing returned was "Not Found," which didn't provide much information for someone trying to determine what failed in the background. Fixes #10160 Signed-off-by: Christopher Kruse --- .changeset/olive-ads-peel.md | 5 ++++ .../actions/builtin/github/helpers.ts | 23 +++++++++++------ .../actions/builtin/publish/github.test.ts | 25 +++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 .changeset/olive-ads-peel.md diff --git a/.changeset/olive-ads-peel.md b/.changeset/olive-ads-peel.md new file mode 100644 index 0000000000..7a7c73ddb7 --- /dev/null +++ b/.changeset/olive-ads-peel.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Provide better error messaging when GitHub fails due to missing team definitions diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts index dae5e16374..25f343da9e 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -190,13 +190,22 @@ export async function createGithubRepoWithCollaboratorsAndTopics( if (access?.startsWith(`${owner}/`)) { const [, team] = access.split('/'); - await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ - org: owner, - team_slug: team, - owner, - repo, - permission: 'admin', - }); + try { + await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ + org: owner, + team_slug: team, + owner, + repo, + permission: 'admin', + }); + } catch (e) { + if (e.data.message === 'Not Found') { + const message = `Received 'Not Found' from the API; one of org: + ${owner}, team: ${team} or repo: ${repo} was not found within GitHub.`; + logger.warn(message); + throw new Error(message, { cause: e }); + } + } // No need to add access if it's the person who owns the personal account } else if (access && access !== owner) { await client.rest.repos.addCollaborator({ diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts index 8ead489ea4..0edeafecfd 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts @@ -518,6 +518,31 @@ describe('publish:github', () => { }); }); + it('should provide an adequate failure message when adding access', async () => { + mockOctokit.rest.users.getByUsername.mockResolvedValue({ + data: { type: 'User' }, + }); + + mockOctokit.rest.repos.createForAuthenticatedUser.mockResolvedValue({ + data: { + clone_url: 'https://github.com/clone/url.git', + html_url: 'https://github.com/html/url', + }, + }); + + mockOctokit.rest.teams.addOrUpdateRepoPermissionsInOrg.mockRejectedValue({ + status: 404, + data: { + message: 'Not Found', + documentation_url: + 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + }, + }); + await expect(action.handler(mockContext)).rejects.toThrow( + "Received 'Not Found' from the API;", + ); + }); + it('should add outside collaborators when provided', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'User' }, From 8cd21b9b596f78758c46a249fd8fde979390c4d2 Mon Sep 17 00:00:00 2001 From: Christopher Kruse Date: Thu, 2 Feb 2023 11:40:44 -0800 Subject: [PATCH 2/3] fix: check for teams before creating repository Shifts the team check for the 'access' parameter to happen prior to the repo creation, so that repositories are not created and have no permissions set on them due to missing teams. Signed-off-by: Christopher Kruse --- .../builtin/github/githubRepoCreate.test.ts | 8 +++ .../actions/builtin/github/helpers.ts | 46 +++++++++------ .../actions/builtin/publish/github.test.ts | 56 ++++++++++++++----- 3 files changed, 81 insertions(+), 29 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts index f15747fd6b..a9ef8c3daa 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts @@ -42,6 +42,7 @@ const mockOctokit = { }, teams: { addOrUpdateRepoPermissionsInOrg: jest.fn(), + getByName: jest.fn(), }, }, }; @@ -96,6 +97,13 @@ describe('github:repo:create', () => { data: { type: 'Organization' }, }); + mockOctokit.rest.teams.getByName.mockResolvedValue({ + data: { + name: 'blam', + id: 42, + }, + }); + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); await action.handler(mockContext); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts index 25f343da9e..dc8c4cd856 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -135,6 +135,10 @@ export async function createGithubRepoWithCollaboratorsAndTopics( username: owner, }); + if (access?.startsWith(`${owner}/`)) { + await validateAccessTeam(client, access); + } + const repoCreationPromise = user.data.type === 'Organization' ? client.rest.repos.createInOrg({ @@ -190,22 +194,13 @@ export async function createGithubRepoWithCollaboratorsAndTopics( if (access?.startsWith(`${owner}/`)) { const [, team] = access.split('/'); - try { - await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ - org: owner, - team_slug: team, - owner, - repo, - permission: 'admin', - }); - } catch (e) { - if (e.data.message === 'Not Found') { - const message = `Received 'Not Found' from the API; one of org: - ${owner}, team: ${team} or repo: ${repo} was not found within GitHub.`; - logger.warn(message); - throw new Error(message, { cause: e }); - } - } + await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ + org: owner, + team_slug: team, + owner, + repo, + permission: 'admin', + }); // No need to add access if it's the person who owns the personal account } else if (access && access !== owner) { await client.rest.repos.addCollaborator({ @@ -360,3 +355,22 @@ function extractCollaboratorName( if ('user' in collaborator) return collaborator.user; return collaborator.team; } + +async function validateAccessTeam(client: Octokit, access: string) { + const [org, team_slug] = access.split('/'); + try { + // Below rule disabled because of a 'getByName' check for a different library + // incorrectly triggers here. + // eslint-disable-next-line testing-library/no-await-sync-query + await client.rest.teams.getByName({ + org, + team_slug, + }); + } catch (e) { + if (e.response.data.message === 'Not Found') { + const message = `Received 'Not Found' from the API; one of org: + ${org} or team: ${team_slug} was not found within GitHub.`; + throw new Error(message, { cause: e }); + } + } +} diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts index 0edeafecfd..94b51f3e94 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts @@ -45,6 +45,7 @@ const mockOctokit = { replaceAllTopics: jest.fn(), }, teams: { + getByName: jest.fn(), addOrUpdateRepoPermissionsInOrg: jest.fn(), }, }, @@ -96,11 +97,41 @@ describe('publish:github', () => { }); }); + it('should fail to create if the team is not found in the org', async () => { + mockOctokit.rest.users.getByUsername.mockResolvedValue({ + data: { type: 'Organization' }, + }); + + mockOctokit.rest.teams.getByName.mockRejectedValue({ + response: { + status: 404, + data: { + message: 'Not Found', + documentation_url: + 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + }, + }, + }); + + await expect(action.handler(mockContext)).rejects.toThrow( + "Received 'Not Found' from the API;", + ); + + expect(mockOctokit.rest.repos.createInOrg).not.toHaveBeenCalled(); + }); + it('should call the githubApis with the correct values for createInOrg', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, }); + mockOctokit.rest.teams.getByName.mockResolvedValue({ + data: { + name: 'blam', + id: 42, + }, + }); + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); await action.handler(mockContext); @@ -523,24 +554,23 @@ describe('publish:github', () => { data: { type: 'User' }, }); - mockOctokit.rest.repos.createForAuthenticatedUser.mockResolvedValue({ - data: { - clone_url: 'https://github.com/clone/url.git', - html_url: 'https://github.com/html/url', - }, - }); - - mockOctokit.rest.teams.addOrUpdateRepoPermissionsInOrg.mockRejectedValue({ - status: 404, - data: { - message: 'Not Found', - documentation_url: - 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + mockOctokit.rest.teams.getByName.mockRejectedValue({ + response: { + status: 404, + data: { + message: 'Not Found', + documentation_url: + 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + }, }, }); await expect(action.handler(mockContext)).rejects.toThrow( "Received 'Not Found' from the API;", ); + + expect( + mockOctokit.rest.repos.createForAuthenticatedUser, + ).not.toHaveBeenCalled(); }); it('should add outside collaborators when provided', async () => { From 67ed63552446b512d549e911e13cd7d36eb36186 Mon Sep 17 00:00:00 2001 From: Christopher Kruse Date: Mon, 13 Feb 2023 10:28:03 -0800 Subject: [PATCH 3/3] fix: use `NotFoundError` for missing team in org Per [PR comment][1], better to use the provided error type that more closely matches the error encountered. [1]: https://github.com/backstage/backstage/pull/16059#discussion_r1103873709 Signed-off-by: Christopher Kruse --- .../src/scaffolder/actions/builtin/github/helpers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts index dc8c4cd856..75c8f25d8b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -15,7 +15,7 @@ */ import { Config } from '@backstage/config'; -import { assertError, InputError } from '@backstage/errors'; +import { assertError, InputError, NotFoundError } from '@backstage/errors'; import { DefaultGithubCredentialsProvider, GithubCredentialsProvider, @@ -370,7 +370,7 @@ async function validateAccessTeam(client: Octokit, access: string) { if (e.response.data.message === 'Not Found') { const message = `Received 'Not Found' from the API; one of org: ${org} or team: ${team_slug} was not found within GitHub.`; - throw new Error(message, { cause: e }); + throw new NotFoundError(message); } } }