From 1fe525a15df53d23b429ef667679b5b68f1b2243 Mon Sep 17 00:00:00 2001 From: Thorsten Lanfer Date: Thu, 12 Sep 2024 13:04:49 +0200 Subject: [PATCH 1/5] Use utility function getClient Signed-off-by: Thorsten Lanfer --- .../actions/gitlabGroupEnsureExists.test.ts | 87 +++++++++++++++++-- .../src/actions/gitlabGroupEnsureExists.ts | 18 ++-- 2 files changed, 85 insertions(+), 20 deletions(-) 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 a92637ba46..67476494bb 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts @@ -25,19 +25,17 @@ const mockGitlabClient = { create: jest.fn(), }, }; -jest.mock('@gitbeaker/node', () => ({ - Gitlab: class { - constructor() { - return mockGitlabClient; - } - }, +// const mockGitlabApi = jest.fn().mockReturnValue(mockGitlabClient); + +jest.mock('../util', () => ({ + getClient: () => mockGitlabClient, })); describe('gitlab:group:ensureExists', () => { const mockContext = createMockActionContext(); afterEach(() => { - jest.resetAllMocks(); + jest.clearAllMocks(); }); it('should create a new group if it does not exists', async () => { @@ -81,7 +79,7 @@ describe('gitlab:group:ensureExists', () => { }); expect(mockGitlabClient.Groups.create).toHaveBeenCalledWith('bar', 'bar', { - parent_id: 2, + parentId: 2, }); expect(mockContext.output).toHaveBeenCalledWith('groupId', 3); @@ -161,4 +159,77 @@ describe('gitlab:group:ensureExists', () => { 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', + }, + ]); + + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'gitlab.com', + token: 'tokenlols', + apiBaseUrl: 'https://api.gitlab.com', + }, + ], + }, + }); + const integrations = ScmIntegrations.fromConfig(config); + const action = createGitlabGroupEnsureExistsAction({ integrations }); + await action.handler({ + ...mockContext, + input: { + repoUrl: 'gitlab.com', + path: ['foobar'], + }, + }); + + // expect(Gitlab).toHaveBeenCalledWith( + // expect.objectContaining({ + // 'token': 'tokenlols', + // }), + // ); + }); + + it('should use a provided token as bearer authentication', async () => { + mockGitlabClient.Groups.search.mockResolvedValue([ + { + id: 1, + full_path: 'foobar', + }, + ]); + + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'gitlab.com', + token: 'tokenlols', + apiBaseUrl: 'https://api.gitlab.com', + }, + ], + }, + }); + const integrations = ScmIntegrations.fromConfig(config); + const action = createGitlabGroupEnsureExistsAction({ integrations }); + await action.handler({ + ...mockContext, + input: { + repoUrl: 'gitlab.com', + path: ['foobar'], + token: 'mysecrettoken', + }, + }); + + // expect(Gitlab).toHaveBeenCalledWith( + // expect.objectContaining({ + // oauthToken: 'mysecrettoken', + // }), + // ); + }); }); diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts index 29f260fb75..31e1d027c2 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts @@ -17,10 +17,9 @@ import { ScmIntegrationRegistry } from '@backstage/integration'; import { createTemplateAction } from '@backstage/plugin-scaffolder-node'; import { GroupSchema } from '@gitbeaker/core/dist/types/resources/Groups'; -import { Gitlab } from '@gitbeaker/node'; import { z } from 'zod'; import commonGitlabConfig from '../commonGitlabConfig'; -import { getToken } from '../util'; +import { getClient } from '../util'; import { examples } from './gitlabGroupEnsureExists.examples'; /** @@ -60,13 +59,8 @@ export const createGitlabGroupEnsureExistsAction = (options: { return; } - const { path } = ctx.input; - const { token, integrationConfig } = getToken(ctx.input, integrations); - - const api = new Gitlab({ - host: integrationConfig.config.baseUrl, - token: token, - }); + const { token, repoUrl, path } = ctx.input; + const api = getClient({ host: repoUrl, token, integrations }); let currentPath: string | null = null; let parent: GroupSchema | null = null; @@ -82,15 +76,15 @@ export const createGitlabGroupEnsureExistsAction = (options: { ); if (!subGroup) { ctx.logger.info(`creating missing group ${fullPath}`); - parent = await api.Groups.create( + parent = (await api.Groups.create( pathElement, pathElement, parent ? { - parent_id: parent.id, + parentId: parent.id, } : {}, - ); + )) as GroupSchema; } else { parent = subGroup; } From cc8d3eafe1bc2855d8592b546bc6ed2e0696450c Mon Sep 17 00:00:00 2001 From: Thorsten Lanfer Date: Thu, 12 Sep 2024 13:29:24 +0200 Subject: [PATCH 2/5] Fix tests Signed-off-by: Thorsten Lanfer --- .../gitlabGroupEnsureExists.examples.test.ts | 13 +- .../actions/gitlabGroupEnsureExists.test.ts | 111 +++++------------- .../src/actions/gitlabGroupEnsureExists.ts | 7 +- 3 files changed, 41 insertions(+), 90 deletions(-) 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 b9377fd97b..202b397d19 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 @@ -27,7 +27,8 @@ const mockGitlabClient = { create: jest.fn(), }, }; -jest.mock('@gitbeaker/node', () => ({ + +jest.mock('@gitbeaker/rest', () => ({ Gitlab: class { constructor() { return mockGitlabClient; @@ -38,8 +39,8 @@ jest.mock('@gitbeaker/node', () => ({ describe('gitlab:group:ensureExists', () => { const mockContext = createMockActionContext(); - afterEach(() => { - jest.resetAllMocks(); + beforeEach(() => { + jest.clearAllMocks(); }); it(`Should ${examples[0].description}`, async () => { @@ -55,7 +56,7 @@ describe('gitlab:group:ensureExists', () => { { host: 'gitlab.com', token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', + apiBaseUrl: 'https://gitlab.com/api/v4', }, ], }, @@ -114,7 +115,7 @@ describe('gitlab:group:ensureExists', () => { 'group2', 'group2', { - parent_id: 1, + parentId: 1, }, ); @@ -161,7 +162,7 @@ describe('gitlab:group:ensureExists', () => { 'group3', 'group3', { - parent_id: 2, + parentId: 2, }, ); 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 67476494bb..b425a4fac8 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts @@ -25,19 +25,37 @@ const mockGitlabClient = { create: jest.fn(), }, }; -// const mockGitlabApi = jest.fn().mockReturnValue(mockGitlabClient); -jest.mock('../util', () => ({ - getClient: () => mockGitlabClient, +jest.mock('@gitbeaker/rest', () => ({ + Gitlab: class { + constructor() { + return mockGitlabClient; + } + }, })); describe('gitlab:group:ensureExists', () => { - const mockContext = createMockActionContext(); - - afterEach(() => { + beforeEach(() => { jest.clearAllMocks(); }); + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'gitlab.com', + token: 'tokenlols', + apiBaseUrl: 'https://gitlab.com/api/v4', + }, + ], + }, + }); + const integrations = ScmIntegrations.fromConfig(config); + + const action = createGitlabGroupEnsureExistsAction({ integrations }); + + const mockContext = createMockActionContext(); + it('should create a new group if it does not exists', async () => { mockGitlabClient.Groups.search.mockResolvedValue([ { @@ -55,25 +73,10 @@ describe('gitlab:group:ensureExists', () => { full_path: 'foo/bar', }); - const config = new ConfigReader({ - integrations: { - gitlab: [ - { - host: 'gitlab.com', - token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', - }, - ], - }, - }); - const integrations = ScmIntegrations.fromConfig(config); - - const action = createGitlabGroupEnsureExistsAction({ integrations }); - await action.handler({ ...mockContext, input: { - repoUrl: 'gitlab.com', + repoUrl: 'gitlab.com?repo=repo&owner=owner', path: ['foo', 'bar'], }, }); @@ -101,25 +104,10 @@ describe('gitlab:group:ensureExists', () => { }, ]); - const config = new ConfigReader({ - integrations: { - gitlab: [ - { - host: 'gitlab.com', - token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', - }, - ], - }, - }); - const integrations = ScmIntegrations.fromConfig(config); - - const action = createGitlabGroupEnsureExistsAction({ integrations }); - await action.handler({ ...mockContext, input: { - repoUrl: 'gitlab.com', + repoUrl: 'gitlab.com?repo=repo&owner=owner', path: ['foo', 'bar'], }, }); @@ -130,26 +118,11 @@ describe('gitlab:group:ensureExists', () => { }); it('should not call API on dryRun', async () => { - const config = new ConfigReader({ - integrations: { - gitlab: [ - { - host: 'gitlab.com', - token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', - }, - ], - }, - }); - const integrations = ScmIntegrations.fromConfig(config); - - const action = createGitlabGroupEnsureExistsAction({ integrations }); - await action.handler({ ...mockContext, isDryRun: true, input: { - repoUrl: 'gitlab.com', + repoUrl: 'gitlab.com?repo=repo&owner=owner', path: ['foo', 'bar'], }, }); @@ -168,23 +141,10 @@ describe('gitlab:group:ensureExists', () => { }, ]); - const config = new ConfigReader({ - integrations: { - gitlab: [ - { - host: 'gitlab.com', - token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', - }, - ], - }, - }); - const integrations = ScmIntegrations.fromConfig(config); - const action = createGitlabGroupEnsureExistsAction({ integrations }); await action.handler({ ...mockContext, input: { - repoUrl: 'gitlab.com', + repoUrl: 'gitlab.com?repo=repo&owner=owner', path: ['foobar'], }, }); @@ -204,23 +164,10 @@ describe('gitlab:group:ensureExists', () => { }, ]); - const config = new ConfigReader({ - integrations: { - gitlab: [ - { - host: 'gitlab.com', - token: 'tokenlols', - apiBaseUrl: 'https://api.gitlab.com', - }, - ], - }, - }); - const integrations = ScmIntegrations.fromConfig(config); - const action = createGitlabGroupEnsureExistsAction({ integrations }); await action.handler({ ...mockContext, input: { - repoUrl: 'gitlab.com', + repoUrl: 'gitlab.com?repo=repo&owner=owner', path: ['foobar'], token: 'mysecrettoken', }, diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts index 31e1d027c2..0a270fc167 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts @@ -19,7 +19,7 @@ import { createTemplateAction } from '@backstage/plugin-scaffolder-node'; import { GroupSchema } from '@gitbeaker/core/dist/types/resources/Groups'; import { z } from 'zod'; import commonGitlabConfig from '../commonGitlabConfig'; -import { getClient } from '../util'; +import { getClient, parseRepoUrl } from '../util'; import { examples } from './gitlabGroupEnsureExists.examples'; /** @@ -60,7 +60,10 @@ export const createGitlabGroupEnsureExistsAction = (options: { } const { token, repoUrl, path } = ctx.input; - const api = getClient({ host: repoUrl, token, integrations }); + + const { host } = parseRepoUrl(repoUrl, integrations); + + const api = getClient({ host, integrations, token }); let currentPath: string | null = null; let parent: GroupSchema | null = null; From f2f68cf31ddd64f445449eb58946477aca721adc Mon Sep 17 00:00:00 2001 From: Thorsten Lanfer Date: Thu, 12 Sep 2024 13:36:15 +0200 Subject: [PATCH 3/5] Add changeset Signed-off-by: Thorsten Lanfer --- .changeset/sour-phones-fix.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/sour-phones-fix.md diff --git a/.changeset/sour-phones-fix.md b/.changeset/sour-phones-fix.md new file mode 100644 index 0000000000..41d35a0c21 --- /dev/null +++ b/.changeset/sour-phones-fix.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-gitlab': patch +--- + +Allow the usage of oauth tokens for the gitlab:group:ensureExists action From e769ae6deb2e001bce19519327fc970aa9d77b17 Mon Sep 17 00:00:00 2001 From: Thorsten Lanfer Date: Tue, 17 Sep 2024 22:01:37 +0200 Subject: [PATCH 4/5] Add tests to check if getClient was used correctly Signed-off-by: Thorsten Lanfer --- .../actions/gitlabGroupEnsureExists.test.ts | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) 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 b425a4fac8..1192052e6d 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.test.ts @@ -18,6 +18,7 @@ import { ConfigReader } from '@backstage/core-app-api'; import { ScmIntegrations } from '@backstage/integration'; import { createMockActionContext } from '@backstage/plugin-scaffolder-node-test-utils'; import { createGitlabGroupEnsureExistsAction } from './gitlabGroupEnsureExists'; +import { getClient } from '../util'; const mockGitlabClient = { Groups: { @@ -34,6 +35,11 @@ jest.mock('@gitbeaker/rest', () => ({ }, })); +jest.mock('../util', () => ({ + getClient: jest.fn().mockImplementation(() => mockGitlabClient), + parseRepoUrl: () => ({ host: 'gitlab.com', owner: 'owner', repo: 'repo' }), +})); + describe('gitlab:group:ensureExists', () => { beforeEach(() => { jest.clearAllMocks(); @@ -149,11 +155,11 @@ describe('gitlab:group:ensureExists', () => { }, }); - // expect(Gitlab).toHaveBeenCalledWith( - // expect.objectContaining({ - // 'token': 'tokenlols', - // }), - // ); + expect(getClient).toHaveBeenCalledWith( + expect.not.objectContaining({ + token: expect.anything(), + }), + ); }); it('should use a provided token as bearer authentication', async () => { @@ -173,10 +179,10 @@ describe('gitlab:group:ensureExists', () => { }, }); - // expect(Gitlab).toHaveBeenCalledWith( - // expect.objectContaining({ - // oauthToken: 'mysecrettoken', - // }), - // ); + expect(getClient).toHaveBeenCalledWith( + expect.objectContaining({ + token: 'mysecrettoken', + }), + ); }); }); From 2834f98cfadd7eda5af9c1df9315fd03e9c80078 Mon Sep 17 00:00:00 2001 From: Thorsten Lanfer Date: Wed, 18 Sep 2024 21:42:12 +0200 Subject: [PATCH 5/5] PR feedback * Use only the ID returned from the group create method, to avoid unnecessary cast * Better changeset Signed-off-by: Thorsten Lanfer --- .changeset/sour-phones-fix.md | 2 +- .../src/actions/gitlabGroupEnsureExists.ts | 28 ++++++++++--------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/.changeset/sour-phones-fix.md b/.changeset/sour-phones-fix.md index 41d35a0c21..d2519f2ef5 100644 --- a/.changeset/sour-phones-fix.md +++ b/.changeset/sour-phones-fix.md @@ -2,4 +2,4 @@ '@backstage/plugin-scaffolder-backend-module-gitlab': patch --- -Allow the usage of oauth tokens for the gitlab:group:ensureExists action +Updated `gitlab:group:ensureExists` action to instead use oauth client. diff --git a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts index 0a270fc167..41bc5755c9 100644 --- a/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts +++ b/plugins/scaffolder-backend-module-gitlab/src/actions/gitlabGroupEnsureExists.ts @@ -66,7 +66,7 @@ export const createGitlabGroupEnsureExistsAction = (options: { const api = getClient({ host, integrations, token }); let currentPath: string | null = null; - let parent: GroupSchema | null = null; + let parentId: number | null = null; for (const pathElement of path) { const fullPath: string = currentPath ? `${currentPath}/${pathElement}` @@ -79,22 +79,24 @@ export const createGitlabGroupEnsureExistsAction = (options: { ); if (!subGroup) { ctx.logger.info(`creating missing group ${fullPath}`); - parent = (await api.Groups.create( - pathElement, - pathElement, - parent - ? { - parentId: parent.id, - } - : {}, - )) as GroupSchema; + parentId = ( + await api.Groups.create( + pathElement, + pathElement, + parentId + ? { + parentId: parentId, + } + : {}, + ) + )?.id; } else { - parent = subGroup; + parentId = subGroup.id; } currentPath = fullPath; } - if (parent !== null) { - ctx.output('groupId', parent?.id); + if (parentId !== null) { + ctx.output('groupId', parentId); } }, });