From 5921b5ce4901b18a70cff5825997570d059718b1 Mon Sep 17 00:00:00 2001 From: Borchies Date: Fri, 23 Sep 2022 12:59:20 +0200 Subject: [PATCH] chore(gitlab-mr-action): remove deprecation of projectid for template action * `projectid` is now passed through the query parameter `project` in the `repoUrl`. It still allows people to not use the `projectid` and use the `repoUrl` with the `owner` and `repo` query parameters instead. This makes it easier to publish to repositories instead of writing the full path to the project. * Changed the `parseUrl` function so it uses a `switch-case` statement which makes the function more readable than having several `if-else` statements. * Created an auxiliary function called `checkRequiredParams`. The function checks if the required parameters (given as arguments) are present in the URL. * The function `parseUrl` uses `checkRequiredParams` to verify that all the required query parameters are present in the repo's URL. This replaces the `if-else` mess with a `switch` statement for better readability. * asserted the type of the constant `repo` to be a string to fix the following error: TS2322 (`Type 'string | null' is not assignable to type 'string'`) * added unit tests in `util.test.ts` for `parseRepoUrl` function Signed-off-by: Borchies --- .changeset/grumpy-clouds-drum.md | 5 + .../builtin/publish/gitlabMergeRequest.ts | 34 +++---- .../actions/builtin/publish/util.test.ts | 96 ++++++++++++++++++- .../actions/builtin/publish/util.ts | 65 ++++++++----- 4 files changed, 157 insertions(+), 43 deletions(-) create mode 100644 .changeset/grumpy-clouds-drum.md diff --git a/.changeset/grumpy-clouds-drum.md b/.changeset/grumpy-clouds-drum.md new file mode 100644 index 0000000000..cf2d5d2a32 --- /dev/null +++ b/.changeset/grumpy-clouds-drum.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': minor +--- + +- The GitLab Project ID for the `publish:gitlab:merge-request` action is now passed through the query parameter `project` in the `repoUrl`. It still allows people to not use the `projectid` and use the `repoUrl` with the `owner` and `repo` query parameters instead. This makes it easier to publish to repositories instead of writing the full path to the project. diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts index 64761f0871..2a71c9f748 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/gitlabMergeRequest.ts @@ -42,7 +42,7 @@ export const createPublishGitlabMergeRequestAction = (options: { targetPath?: string; token?: string; commitAction?: 'create' | 'delete' | 'update'; - /** @deprecated Use projectPath instead */ + /** @deprecated projectID passed as query parameters in the repoUrl */ projectid?: string; removeSourceBranch?: boolean; assignee?: string; @@ -58,7 +58,7 @@ export const createPublishGitlabMergeRequestAction = (options: { title: 'Repository Location', description: `Accepts the format 'gitlab.com/group_name/project_name' where 'project_name' is the repository name and 'group_name' is a group or username`, }, - /** @deprecated Use projectPath instead */ + /** @deprecated projectID is passed as query parameters in the repoUrl */ projectid: { type: 'string', title: 'projectid', @@ -146,14 +146,12 @@ export const createPublishGitlabMergeRequestAction = (options: { title, token: providedToken, } = ctx.input; - const { host, owner, repo } = parseRepoUrl(repoUrl, integrations); - const projectPath = `${owner}/${repo}`; - if (ctx.input.projectid) { - const deprecationWarning = `Property "projectid" is deprecated and no longer to needed to create a MR`; - ctx.logger.warn(deprecationWarning); - console.warn(deprecationWarning); - } + const { host, owner, repo, project } = parseRepoUrl( + repoUrl, + integrations, + ); + const repoID = project ? project : `${owner}/${repo}`; const integrationConfig = integrations.gitlab.byHost(host); @@ -211,22 +209,19 @@ export const createPublishGitlabMergeRequestAction = (options: { content: file.content.toString('base64'), execute_filemode: file.executable, })); - const projects = await api.Projects.show(projectPath); + + const projects = await api.Projects.show(repoID); const { default_branch: defaultBranch } = projects; try { - await api.Branches.create( - projectPath, - branchName, - String(defaultBranch), - ); + await api.Branches.create(repoID, branchName, String(defaultBranch)); } catch (e) { throw new InputError(`The branch creation failed ${e}`); } try { - await api.Commits.create(projectPath, branchName, title, actions); + await api.Commits.create(repoID, branchName, ctx.input.title, actions); } catch (e) { throw new InputError( `Committing the changes to ${branchName} failed ${e}`, @@ -235,7 +230,7 @@ export const createPublishGitlabMergeRequestAction = (options: { try { const mergeRequestUrl = await api.MergeRequests.create( - projectPath, + repoID, branchName, String(defaultBranch), title, @@ -247,9 +242,8 @@ export const createPublishGitlabMergeRequestAction = (options: { ).then((mergeRequest: { web_url: string }) => { return mergeRequest.web_url; }); - /** @deprecated */ - ctx.output('projectid', projectPath); - ctx.output('projectPath', projectPath); + ctx.output('projectid', repoID); + ctx.output('projectPath', repoID); ctx.output('mergeRequestUrl', mergeRequestUrl); } catch (e) { throw new InputError(`Merge request creation failed${e}`); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts index cc879d4f55..5446109056 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.test.ts @@ -15,7 +15,9 @@ */ import path from 'path'; -import { getRepoSourceDirectory, isExecutable } from './util'; +import { getRepoSourceDirectory, isExecutable, parseRepoUrl } from './util'; +import { ScmIntegrations } from '@backstage/integration'; +import { ConfigReader } from '@backstage/config'; describe('getRepoSourceDirectory', () => { it('should return workspace root if no sub folder is given', () => { @@ -85,3 +87,95 @@ describe('isExecutable', () => { expect(isExecutable(0o100640)).toBe(false); }); }); + +describe('parseRepoUrl', () => { + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'www.gitlab.com', + token: 'token', + apiBaseUrl: 'https://api.gitlab.com', + }, + ], + bitbucket: [ + { + host: 'www.bitbucket.net', + apiBaseUrl: 'https://api.hosted.bitbucket.net', + }, + { + host: 'www.bitbucket.org', + username: 'username', + appPassword: 'password', + apiBaseUrl: 'https://api.hosted.bitbucket.org', + }, + ], + }, + }); + const integrations = ScmIntegrations.fromConfig(config); + it('should throw an exception if no parameters are passed (gitlab)', () => { + expect(() => parseRepoUrl('www.gitlab.com', integrations)).toThrow( + 'Invalid repo URL passed to publisher: https://www.gitlab.com/, missing owner', + ); + }); + it('should return the project ID in the `project` parameter (gitlab)', () => { + expect( + parseRepoUrl('www.gitlab.com?project=234', integrations), + ).toStrictEqual({ + host: 'www.gitlab.com', + organization: undefined, + owner: undefined, + project: '234', + repo: null, + workspace: undefined, + }); + }); + it('should throw an exception if the repo parameter is missing, but owner parameter is set', () => { + expect(() => + parseRepoUrl('www.gitlab.com?owner=owner', integrations), + ).toThrow( + 'Invalid repo URL passed to publisher: https://www.gitlab.com/?owner=owner, missing repo', + ); + }); + it('should throw an exception if the owner parameter is missing, but repo parameter is set (gitlab)', () => { + expect(() => + parseRepoUrl('www.gitlab.com?repo=repo', integrations), + ).toThrow( + 'Invalid repo URL passed to publisher: https://www.gitlab.com/?repo=repo, missing owner', + ); + }); + it('should return the workspace, project and repo (bitbucket.org)', () => { + expect( + parseRepoUrl( + 'www.bitbucket.org?project=project&workspace=workspace&repo=repo', + integrations, + ), + ).toStrictEqual({ + host: 'www.bitbucket.org', + organization: undefined, + owner: undefined, + project: 'project', + repo: 'repo', + workspace: 'workspace', + }); + }); + it('should return the project and repo (another bitbucket instance)', () => { + expect( + parseRepoUrl('www.bitbucket.net?project=project&repo=repo', integrations), + ).toStrictEqual({ + host: 'www.bitbucket.net', + organization: undefined, + owner: undefined, + project: 'project', + repo: 'repo', + workspace: undefined, + }); + }); + it('should throw an exception if the workspace parameter is missing for bitbucket.org instance (bitbucket.org)', () => { + expect(() => + parseRepoUrl('www.bitbucket.org?project=project&repo=repo', integrations), + ).toThrow( + 'Invalid repo URL passed to publisher: https://www.bitbucket.org/?project=project&repo=repo, missing workspace', + ); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts index fff3213cb7..61bd704a0b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/util.ts @@ -71,32 +71,30 @@ export const parseRepoUrl = ( ); } - if (type === 'bitbucket') { - if (host === 'bitbucket.org') { - if (!workspace) { - throw new InputError( - `Invalid repo URL passed to publisher: ${repoUrl}, missing workspace`, - ); + const repo: string = parsed.searchParams.get('repo')!; + switch (type) { + case 'bitbucket': { + if (host === 'www.bitbucket.org') { + checkRequiredParams(parsed, 'workspace'); } + checkRequiredParams(parsed, 'project', 'repo'); + break; } - if (!project) { - throw new InputError( - `Invalid repo URL passed to publisher: ${repoUrl}, missing project`, - ); + case 'gitlab': { + // project is the projectID, and if defined, owner and repo won't be needed. + if (!project) { + checkRequiredParams(parsed, 'owner', 'repo'); + } + break; } - } else { - if (!owner && type !== 'gerrit') { - throw new InputError( - `Invalid repo URL passed to publisher: ${repoUrl}, missing owner`, - ); + case 'gerrit': { + checkRequiredParams(parsed, 'repo'); + break; + } + default: { + checkRequiredParams(parsed, 'repo', 'owner'); + break; } - } - - const repo = parsed.searchParams.get('repo'); - if (!repo) { - throw new InputError( - `Invalid repo URL passed to publisher: ${repoUrl}, missing repo`, - ); } return { host, owner, repo, organization, workspace, project }; @@ -106,3 +104,26 @@ export const isExecutable = (fileMode: number) => { const res = fileMode & executeBitMask; return res > 0; }; + +/** + * This function checks if the required parameters (given as the `params` argument) are present in the URL + * + * @param repoUrl - the URL for the repository + * @param params - a variadic list of URL query parameter names + * + * @throws + * An InputError exception is thrown if any of the required parameters is not in the URL. + * + * @public + */ +function checkRequiredParams(repoUrl: URL, ...params: string[]) { + for (let i = 0; i < params.length; i++) { + if (!repoUrl.searchParams.get(params[i])) { + throw new InputError( + `Invalid repo URL passed to publisher: ${repoUrl.toString()}, missing ${ + params[i] + }`, + ); + } + } +}