From 8f0bd33d7a9ac34481fc9bca952634c3a19fc92e Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 27 Mar 2023 11:59:38 -0500 Subject: [PATCH 1/7] added utility to remove backstage entity refs from entities Signed-off-by: Phred --- .../actions/builtin/helpers.test.ts | 21 ++++++++++++++++++- .../src/scaffolder/actions/builtin/helpers.ts | 4 ++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts index b5a375d83d..47f2c88b30 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts @@ -15,7 +15,12 @@ */ import { Git, getVoidLogger } from '@backstage/backend-common'; -import { commitAndPushRepo, initRepoAndPush } from './helpers'; +import { entityNamePickerValidation } from '@backstage/plugin-scaffolder/src/components/fields/EntityNamePicker'; +import { + commitAndPushRepo, + familiarizeEntityName, + initRepoAndPush, +} from './helpers'; jest.mock('@backstage/backend-common', () => ({ Git: { @@ -301,3 +306,17 @@ describe('commitAndPushRepo', () => { }); }); }); + +describe('familiarizeEntityName', () => { + it.each([ + 'user:default/catpants', + 'group:default/catpants', + 'user:catpants', + 'default/catpants', + 'user:custom/catpants', + 'group:custom/catpants', + 'catpants', + ])('should parse: "%s"', (entityName: string) => { + expect(familiarizeEntityName(entityName)).toEqual('catpants'); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts index 8bed279a1b..30b75d9e6f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts @@ -296,3 +296,7 @@ export function getGitCommitMessage( ? gitCommitMessage : config.getOptionalString('scaffolder.defaultCommitMessage'); } + +export function familiarizeEntityName(name: string): string { + return name.replace(/^.*[:/]/g, ''); +} From 57a49da161cfd17dad67c1aa17c7bd346aebc857 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 27 Mar 2023 11:59:42 -0500 Subject: [PATCH 2/7] use helper to familiarize entities passed to GitHub Signed-off-by: Phred --- .../actions/builtin/github/helpers.ts | 5 +- .../actions/builtin/publish/github.test.ts | 61 +++++++++++++++++++ 2 files changed, 64 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 75c8f25d8b..23f6b804b7 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -29,6 +29,7 @@ import { initRepoAndPush, } from '../helpers'; import { getRepoSourceDirectory, parseRepoUrl } from '../publish/util'; +import { familiarizeEntityName } from '../../builtin/helpers'; const DEFAULT_TIMEOUT_MS = 60_000; @@ -218,13 +219,13 @@ export async function createGithubRepoWithCollaboratorsAndTopics( await client.rest.repos.addCollaborator({ owner, repo, - username: collaborator.user, + username: familiarizeEntityName(collaborator.user), permission: collaborator.access, }); } else if ('team' in collaborator) { await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ org: owner, - team_slug: collaborator.team, + team_slug: familiarizeEntityName(collaborator.team), owner, repo, permission: collaborator.access, 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 94b51f3e94..4bd8ee1652 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 @@ -29,6 +29,7 @@ import { when } from 'jest-when'; import { PassThrough } from 'stream'; import { enableBranchProtectionOnDefaultRepoBranch, + familiarizeEntityName, initRepoAndPush, } from '../helpers'; import { createPublishGithubAction } from './github'; @@ -68,6 +69,8 @@ describe('publish:github', () => { }, }); + const { familiarizeEntityName: realFamiliarizeEntityName } = + jest.requireActual('../helpers'); const integrations = ScmIntegrations.fromConfig(config); let githubCredentialsProvider: GithubCredentialsProvider; let action: TemplateAction; @@ -95,6 +98,11 @@ describe('publish:github', () => { config, githubCredentialsProvider, }); + + // restore real implmentation + (familiarizeEntityName as jest.MockedFunction).mockImplementation( + realFamiliarizeEntityName, + ); }); it('should fail to create if the team is not found in the org', async () => { @@ -651,6 +659,59 @@ describe('publish:github', () => { }); }); + it('should familiarize entity names while adding collaborators', 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', + }, + }); + + await action.handler({ + ...mockContext, + input: { + ...mockContext.input, + collaborators: [ + { + access: 'pull', + user: 'user:robot-1', + }, + { + access: 'push', + team: 'group:default/robot-2', + }, + ], + }, + }); + + const commonProperties = { + owner: 'owner', + repo: 'repo', + }; + + expect(mockOctokit.rest.repos.addCollaborator).toHaveBeenCalledWith({ + ...commonProperties, + username: 'robot-1', + permission: 'pull', + }); + + expect( + mockOctokit.rest.teams.addOrUpdateRepoPermissionsInOrg, + ).toHaveBeenCalledWith({ + ...commonProperties, + org: 'owner', + team_slug: 'robot-2', + permission: 'push', + }); + + expect(familiarizeEntityName).toHaveBeenCalledWith('user:robot-1'); + expect(familiarizeEntityName).toHaveBeenCalledWith('group:default/robot-2'); + }); + it('should ignore failures when adding multiple collaborators', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'User' }, From f37a95adcd8a9a2ad8e5b599706aeba48973a796 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 27 Mar 2023 11:59:47 -0500 Subject: [PATCH 3/7] added changeset Signed-off-by: Phred --- .changeset/serious-items-walk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/serious-items-walk.md diff --git a/.changeset/serious-items-walk.md b/.changeset/serious-items-walk.md new file mode 100644 index 0000000000..6da6d91ac6 --- /dev/null +++ b/.changeset/serious-items-walk.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Removed entity types and namespacing before passing to GitHub API From abbcb55554623639a0425a36c18ba3ad88676db7 Mon Sep 17 00:00:00 2001 From: Phred Date: Mon, 27 Mar 2023 12:25:59 -0500 Subject: [PATCH 4/7] cleaened up tsc issues Signed-off-by: Phred --- .../src/scaffolder/actions/builtin/helpers.test.ts | 1 - .../src/scaffolder/actions/builtin/publish/github.test.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts index 47f2c88b30..8cde257d64 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts @@ -15,7 +15,6 @@ */ import { Git, getVoidLogger } from '@backstage/backend-common'; -import { entityNamePickerValidation } from '@backstage/plugin-scaffolder/src/components/fields/EntityNamePicker'; import { commitAndPushRepo, familiarizeEntityName, 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 4bd8ee1652..82f6948fa8 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 @@ -100,7 +100,7 @@ describe('publish:github', () => { }); // restore real implmentation - (familiarizeEntityName as jest.MockedFunction).mockImplementation( + (familiarizeEntityName as jest.Mock).mockImplementation( realFamiliarizeEntityName, ); }); From 88627d957ea85a35b901e63dbe17be7c57f312dc Mon Sep 17 00:00:00 2001 From: Phred Date: Tue, 28 Mar 2023 08:07:50 -0500 Subject: [PATCH 5/7] mocked helper in github repo create tests Signed-off-by: Phred --- .../actions/builtin/github/githubRepoCreate.test.ts | 5 ++++- .../src/scaffolder/actions/builtin/publish/github.test.ts | 3 ++- 2 files changed, 6 insertions(+), 2 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 a9ef8c3daa..58747d57ef 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 @@ -28,6 +28,7 @@ import { import { when } from 'jest-when'; import { PassThrough } from 'stream'; import { createGithubRepoCreateAction } from './githubRepoCreate'; +import { familiarizeEntityName } from '../helpers'; const mockOctokit = { rest: { @@ -83,15 +84,17 @@ describe('github:repo:create', () => { }; beforeEach(() => { - jest.resetAllMocks(); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createGithubRepoCreateAction({ integrations, githubCredentialsProvider, }); + (familiarizeEntityName as jest.Mock).mockImplementation((s: string) => s); }); + afterEach(jest.resetAllMocks); + it('should call the githubApis with the correct values for createInOrg', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, 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 82f6948fa8..3347ca2e05 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 @@ -90,7 +90,6 @@ describe('publish:github', () => { }; beforeEach(() => { - jest.resetAllMocks(); githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); action = createPublishGithubAction({ @@ -105,6 +104,8 @@ describe('publish:github', () => { ); }); + afterEach(jest.resetAllMocks); + it('should fail to create if the team is not found in the org', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, From 4933f965d525474a1d61b88b2d43ca33601772d6 Mon Sep 17 00:00:00 2001 From: Phred Date: Tue, 28 Mar 2023 18:03:51 -0400 Subject: [PATCH 6/7] made changeset more explicit Signed-off-by: Phred --- .changeset/serious-items-walk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/serious-items-walk.md b/.changeset/serious-items-walk.md index 6da6d91ac6..c4cdfe34c4 100644 --- a/.changeset/serious-items-walk.md +++ b/.changeset/serious-items-walk.md @@ -2,4 +2,4 @@ '@backstage/plugin-scaffolder-backend': patch --- -Removed entity types and namespacing before passing to GitHub API +Stripped entity types and namespace before passing to GitHub API From 98d06e02aafe88d66407ce3bacc894260097b573 Mon Sep 17 00:00:00 2001 From: Phred Date: Tue, 4 Apr 2023 09:16:18 -0500 Subject: [PATCH 7/7] renamed utility to entityRefToName based on code review Signed-off-by: Phred --- .../actions/builtin/github/githubRepoCreate.test.ts | 4 ++-- .../src/scaffolder/actions/builtin/github/helpers.ts | 6 +++--- .../src/scaffolder/actions/builtin/helpers.test.ts | 10 +++------- .../src/scaffolder/actions/builtin/helpers.ts | 2 +- .../scaffolder/actions/builtin/publish/github.test.ts | 10 +++++----- 5 files changed, 14 insertions(+), 18 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 58747d57ef..4f23839fca 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 @@ -28,7 +28,7 @@ import { import { when } from 'jest-when'; import { PassThrough } from 'stream'; import { createGithubRepoCreateAction } from './githubRepoCreate'; -import { familiarizeEntityName } from '../helpers'; +import { entityRefToName } from '../helpers'; const mockOctokit = { rest: { @@ -90,7 +90,7 @@ describe('github:repo:create', () => { integrations, githubCredentialsProvider, }); - (familiarizeEntityName as jest.Mock).mockImplementation((s: string) => s); + (entityRefToName as jest.Mock).mockImplementation((s: string) => s); }); afterEach(jest.resetAllMocks); 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 23f6b804b7..57be72bc73 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -29,7 +29,7 @@ import { initRepoAndPush, } from '../helpers'; import { getRepoSourceDirectory, parseRepoUrl } from '../publish/util'; -import { familiarizeEntityName } from '../../builtin/helpers'; +import { entityRefToName } from '../../builtin/helpers'; const DEFAULT_TIMEOUT_MS = 60_000; @@ -219,13 +219,13 @@ export async function createGithubRepoWithCollaboratorsAndTopics( await client.rest.repos.addCollaborator({ owner, repo, - username: familiarizeEntityName(collaborator.user), + username: entityRefToName(collaborator.user), permission: collaborator.access, }); } else if ('team' in collaborator) { await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ org: owner, - team_slug: familiarizeEntityName(collaborator.team), + team_slug: entityRefToName(collaborator.team), owner, repo, permission: collaborator.access, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts index 8cde257d64..239f7c7351 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.test.ts @@ -15,11 +15,7 @@ */ import { Git, getVoidLogger } from '@backstage/backend-common'; -import { - commitAndPushRepo, - familiarizeEntityName, - initRepoAndPush, -} from './helpers'; +import { commitAndPushRepo, entityRefToName, initRepoAndPush } from './helpers'; jest.mock('@backstage/backend-common', () => ({ Git: { @@ -306,7 +302,7 @@ describe('commitAndPushRepo', () => { }); }); -describe('familiarizeEntityName', () => { +describe('entityRefToName', () => { it.each([ 'user:default/catpants', 'group:default/catpants', @@ -316,6 +312,6 @@ describe('familiarizeEntityName', () => { 'group:custom/catpants', 'catpants', ])('should parse: "%s"', (entityName: string) => { - expect(familiarizeEntityName(entityName)).toEqual('catpants'); + expect(entityRefToName(entityName)).toEqual('catpants'); }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts index 30b75d9e6f..c1630d0bf7 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/helpers.ts @@ -297,6 +297,6 @@ export function getGitCommitMessage( : config.getOptionalString('scaffolder.defaultCommitMessage'); } -export function familiarizeEntityName(name: string): string { +export function entityRefToName(name: string): string { return name.replace(/^.*[:/]/g, ''); } 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 3347ca2e05..8c6b02c1c3 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 @@ -29,7 +29,7 @@ import { when } from 'jest-when'; import { PassThrough } from 'stream'; import { enableBranchProtectionOnDefaultRepoBranch, - familiarizeEntityName, + entityRefToName, initRepoAndPush, } from '../helpers'; import { createPublishGithubAction } from './github'; @@ -69,7 +69,7 @@ describe('publish:github', () => { }, }); - const { familiarizeEntityName: realFamiliarizeEntityName } = + const { entityRefToName: realFamiliarizeEntityName } = jest.requireActual('../helpers'); const integrations = ScmIntegrations.fromConfig(config); let githubCredentialsProvider: GithubCredentialsProvider; @@ -99,7 +99,7 @@ describe('publish:github', () => { }); // restore real implmentation - (familiarizeEntityName as jest.Mock).mockImplementation( + (entityRefToName as jest.Mock).mockImplementation( realFamiliarizeEntityName, ); }); @@ -709,8 +709,8 @@ describe('publish:github', () => { permission: 'push', }); - expect(familiarizeEntityName).toHaveBeenCalledWith('user:robot-1'); - expect(familiarizeEntityName).toHaveBeenCalledWith('group:default/robot-2'); + expect(entityRefToName).toHaveBeenCalledWith('user:robot-1'); + expect(entityRefToName).toHaveBeenCalledWith('group:default/robot-2'); }); it('should ignore failures when adding multiple collaborators', async () => {