diff --git a/.changeset/easy-deer-eat.md b/.changeset/easy-deer-eat.md new file mode 100644 index 0000000000..10f21e9b29 --- /dev/null +++ b/.changeset/easy-deer-eat.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': patch +--- + +Changed `gitlab:group:ensureExists` action to use `Groups.show` API instead of `Groups.search` for checking if a group path exists. This is more efficient as it directly retrieves the group by path rather than searching and filtering results. diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.examples.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.examples.test.ts index 449c5980a4..c665676daa 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.examples.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.examples.test.ts @@ -23,7 +23,7 @@ import { mockServices } from '@backstage/backend-test-utils'; const mockGitlabClient = { Groups: { - search: jest.fn(), + show: jest.fn(), create: jest.fn(), }, }; @@ -44,7 +44,9 @@ describe('gitlab:group:ensureExists', () => { }); it(`Should ${examples[0].description}`, async () => { - mockGitlabClient.Groups.search.mockResolvedValue([]); + mockGitlabClient.Groups.show.mockRejectedValue({ + cause: { response: { status: 404 } }, + }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, full_path: 'group1', @@ -82,12 +84,9 @@ describe('gitlab:group:ensureExists', () => { }); it(`Should ${examples[1].description}`, async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'group1', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 1, full_path: 'group1' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, full_path: 'group1/group2', @@ -127,16 +126,10 @@ describe('gitlab:group:ensureExists', () => { }); it(`Should ${examples[2].description}`, async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'group1', - }, - { - id: 2, - full_path: 'group1/group2', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 1, full_path: 'group1' }) + .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, full_path: 'group1/group2/group3', @@ -199,23 +192,17 @@ describe('gitlab:group:ensureExists', () => { input: yaml.parse(examples[3].example).steps[0].input, }); - expect(mockGitlabClient.Groups.search).not.toHaveBeenCalled(); + expect(mockGitlabClient.Groups.show).not.toHaveBeenCalled(); expect(mockGitlabClient.Groups.create).not.toHaveBeenCalled(); expect(mockContext.output).toHaveBeenCalledWith('groupId', 42); }); it(`Should ${examples[4].description}`, async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'group1', - }, - { - id: 2, - full_path: 'group1/group2', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 1, full_path: 'group1' }) + .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, full_path: 'group1/group2/group3', @@ -255,20 +242,11 @@ describe('gitlab:group:ensureExists', () => { }); it(`Should ${examples[5].description}`, async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'group1', - }, - { - id: 2, - full_path: 'group1/group2', - }, - { - id: 3, - full_path: 'group1/group2/group3', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 1, full_path: 'group1' }) + .mockResolvedValueOnce({ id: 2, full_path: 'group1/group2' }) + .mockResolvedValueOnce({ id: 3, full_path: 'group1/group2/group3' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 4, full_path: 'group1/group2/group3/group4', diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts index 8e52ee999e..73f877eae7 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts @@ -22,7 +22,7 @@ import { mockServices } from '@backstage/backend-test-utils'; const mockGitlabClient = { Groups: { - search: jest.fn(), + show: jest.fn(), create: jest.fn(), }, }; @@ -65,16 +65,9 @@ describe('gitlab:group:ensureExists', () => { const mockContext = createMockActionContext(); it('should create a new group from string if it does not exists', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'bar', - }, - { - id: 2, - full_path: 'foo', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 2, full_path: 'foo' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, @@ -97,21 +90,14 @@ describe('gitlab:group:ensureExists', () => { }); it('should create a new group from pathstring if it does not exists', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'bar', - }, - { - id: 2, - full_path: 'foo', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 2, full_path: 'foo' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); - mockGitlabClient.Groups.create.mockResolvedValue({ - id: 3, - full_path: 'foo/bar', - }); + mockGitlabClient.Groups.create + .mockResolvedValueOnce({ id: 3, full_path: 'foo/bar' }) + .mockResolvedValueOnce({ id: 4, full_path: 'foo/bar/baz' }); await action.handler({ ...mockContext, @@ -121,24 +107,31 @@ describe('gitlab:group:ensureExists', () => { }, }); - expect(mockGitlabClient.Groups.create).toHaveBeenCalledWith('bar', 'bar', { - parentId: 2, - }); + expect(mockGitlabClient.Groups.create).toHaveBeenNthCalledWith( + 1, + 'bar', + 'bar', + { + parentId: 2, + }, + ); - expect(mockContext.output).toHaveBeenCalledWith('groupId', 3); + expect(mockGitlabClient.Groups.create).toHaveBeenNthCalledWith( + 2, + 'baz', + 'baz', + { + parentId: 3, + }, + ); + + expect(mockContext.output).toHaveBeenCalledWith('groupId', 4); }); it('should create a new group from object if it does not exists', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'bar', - }, - { - id: 2, - full_path: 'foo', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 2, full_path: 'foo' }) + .mockRejectedValueOnce({ cause: { response: { status: 404 } } }); mockGitlabClient.Groups.create.mockResolvedValue({ id: 3, @@ -168,20 +161,9 @@ describe('gitlab:group:ensureExists', () => { }); it('should return existing group if it does exists', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'bar', - }, - { - id: 2, - full_path: 'foo', - }, - { - id: 42, - full_path: 'foo/bar', - }, - ]); + mockGitlabClient.Groups.show + .mockResolvedValueOnce({ id: 2, full_path: 'foo' }) + .mockResolvedValueOnce({ id: 42, full_path: 'foo/bar' }); await action.handler({ ...mockContext, @@ -206,19 +188,17 @@ describe('gitlab:group:ensureExists', () => { }, }); - expect(mockGitlabClient.Groups.search).not.toHaveBeenCalled(); + expect(mockGitlabClient.Groups.show).not.toHaveBeenCalled(); expect(mockGitlabClient.Groups.create).not.toHaveBeenCalled(); expect(mockContext.output).toHaveBeenCalledWith('groupId', 42); }); it('should use the token from the integration config when none is provided', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'foobar', - }, - ]); + mockGitlabClient.Groups.show.mockResolvedValue({ + id: 1, + full_path: 'foobar', + }); await action.handler({ ...mockContext, @@ -236,12 +216,10 @@ describe('gitlab:group:ensureExists', () => { }); it('should use a provided token as bearer authentication', async () => { - mockGitlabClient.Groups.search.mockResolvedValue([ - { - id: 1, - full_path: 'foobar', - }, - ]); + mockGitlabClient.Groups.show.mockResolvedValue({ + id: 1, + full_path: 'foobar', + }); await action.handler({ ...mockContext, diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts index c3d4fd74bf..590437fd8e 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts @@ -88,12 +88,18 @@ export const createGitlabGroupEnsureExistsAction = (options: { let parentId: number | null = null; for (const { name, slug } of pathIterator(path)) { const fullPath: string = currentPath ? `${currentPath}/${slug}` : slug; - const result = (await api.Groups.search( - fullPath, - )) as unknown as Array; // recast since the return type for search is wrong in the gitbeaker typings - const subGroup = result.find( - searchPathElem => searchPathElem.full_path === fullPath, - ); + let subGroup: GroupSchema | undefined; + try { + subGroup = (await api.Groups.show( + fullPath, + )) as unknown as GroupSchema; + } catch (error: any) { + // 404 means the group doesn't exist - we'll create it below + // Any other error should be thrown + if (error.cause?.response?.status !== 404) { + throw error; + } + } if (!subGroup) { ctx.logger.info(`creating missing group ${fullPath}`);