From ffffea8e6d2dccc312343192bfbe26fe095bb292 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 8 Feb 2021 10:52:10 +0100 Subject: [PATCH] integration: update the gitlab config mandatory fields to match reality --- .changeset/great-waves-reflect.md | 6 +++ .changeset/pink-kids-bake.md | 31 +++++++++++++ .../src/reading/GitlabUrlReader.test.ts | 14 +----- .../src/reading/GitlabUrlReader.ts | 6 --- .../src/gitlab/GitLabIntegration.test.ts | 2 + .../integration/src/gitlab/config.test.ts | 10 +++- packages/integration/src/gitlab/config.ts | 46 ++++++++++++++----- packages/integration/src/gitlab/core.test.ts | 4 ++ packages/integration/src/helpers.ts | 19 ++++++-- .../scaffolder/stages/prepare/gitlab.test.ts | 4 +- .../scaffolder/stages/publish/gitlab.test.ts | 3 ++ .../scaffolder/stages/publish/publishers.ts | 2 + 12 files changed, 111 insertions(+), 36 deletions(-) create mode 100644 .changeset/great-waves-reflect.md create mode 100644 .changeset/pink-kids-bake.md diff --git a/.changeset/great-waves-reflect.md b/.changeset/great-waves-reflect.md new file mode 100644 index 0000000000..9304a51deb --- /dev/null +++ b/.changeset/great-waves-reflect.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-common': patch +'@backstage/plugin-scaffolder-backend': patch +--- + +Minor updates to reflect the changes in `@backstage/integration` that made the fields `apiBaseUrl` and `apiUrl` mandatory. diff --git a/.changeset/pink-kids-bake.md b/.changeset/pink-kids-bake.md new file mode 100644 index 0000000000..c0bd846ff1 --- /dev/null +++ b/.changeset/pink-kids-bake.md @@ -0,0 +1,31 @@ +--- +'@backstage/integration': minor +--- + +Update the `GitLabIntegrationConfig` to require the fields `apiBaseUrl` and `baseUrl`. The `readGitLabIntegrationConfig` function is now more strict and has better error reporting. This change mirrors actual reality in code more properly - the fields are actually necessary for many parts of code to actually function, so they should no longer be optional. + +Some checks that used to happen deep inside code that consumed config, now happen upfront at startup. This means that you may encounter new errors at backend startup, if you had actual mistakes in config but didn't happen to exercise the code paths that actually would break. But for most users, no change will be necessary. + +An example minimal GitLab config block that just adds a token to public GitLab would look similar to this: + +```yaml +integrations: + gitlab: + - host: gitlab.com + token: + $env: GITLAB_TOKEN +``` + +A full fledged config that points to a locally hosted GitLab could look like this: + +```yaml +integrations: + gitlab: + - host: gitlab.my-company.com + apiBaseUrl: https://gitlab.my-company.com/api/v4 + baseUrl: https://gitlab.my-company.com + token: + $env: OUR_GITLAB_TOKEN +``` + +In this case, the only optional field is `baseUrl` which is formed from the `host` if needed. diff --git a/packages/backend-common/src/reading/GitlabUrlReader.test.ts b/packages/backend-common/src/reading/GitlabUrlReader.test.ts index b18e7a4294..5b0b4002a3 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.test.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.test.ts @@ -36,6 +36,7 @@ const gitlabProcessor = new GitlabUrlReader( { host: 'gitlab.com', apiBaseUrl: 'https://gitlab.com/api/v4', + baseUrl: 'https://gitlab.com', }, { treeResponseFactory }, ); @@ -44,6 +45,7 @@ const hostedGitlabProcessor = new GitlabUrlReader( { host: 'gitlab.mycompany.com', apiBaseUrl: 'https://gitlab.mycompany.com/api/v4', + baseUrl: 'https://gitlab.mycompany.com', }, { treeResponseFactory }, ); @@ -379,17 +381,5 @@ describe('GitlabUrlReader', () => { }; await expect(fnGithub).rejects.toThrow(NotFoundError); }); - - it('should throw error when apiBaseUrl is missing', () => { - expect(() => { - /* eslint-disable no-new */ - new GitlabUrlReader( - { - host: 'gitlab.mycompany.com', - }, - { treeResponseFactory }, - ); - }).toThrowError('must configure an explicit apiBaseUrl'); - }); }); }); diff --git a/packages/backend-common/src/reading/GitlabUrlReader.ts b/packages/backend-common/src/reading/GitlabUrlReader.ts index 8a763d1259..22316f6895 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.ts @@ -51,12 +51,6 @@ export class GitlabUrlReader implements UrlReader { deps: { treeResponseFactory: ReadTreeResponseFactory }, ) { this.treeResponseFactory = deps.treeResponseFactory; - - if (!config.apiBaseUrl) { - throw new Error( - `GitLab integration for '${config.host}' must configure an explicit apiBaseUrl`, - ); - } } async read(url: string): Promise { diff --git a/packages/integration/src/gitlab/GitLabIntegration.test.ts b/packages/integration/src/gitlab/GitLabIntegration.test.ts index 8814e33302..6faae3a5e7 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.test.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.test.ts @@ -26,6 +26,8 @@ describe('GitLabIntegration', () => { { host: 'h.com', token: 't', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', }, ], }, diff --git a/packages/integration/src/gitlab/config.test.ts b/packages/integration/src/gitlab/config.test.ts index 303b4bd270..bfa59d001e 100644 --- a/packages/integration/src/gitlab/config.test.ts +++ b/packages/integration/src/gitlab/config.test.ts @@ -31,6 +31,7 @@ describe('readGitLabIntegrationConfig', () => { buildConfig({ host: 'a.com', token: 't', + apiBaseUrl: 'https://a.com', baseUrl: 'https://baseurl.for.me/gitlab', }), ); @@ -38,12 +39,15 @@ describe('readGitLabIntegrationConfig', () => { expect(output).toEqual({ host: 'a.com', token: 't', + apiBaseUrl: 'https://a.com', baseUrl: 'https://baseurl.for.me/gitlab', }); }); it('inserts the defaults if missing', () => { - const output = readGitLabIntegrationConfig(buildConfig({})); + const output = readGitLabIntegrationConfig( + buildConfig({ host: 'gitlab.com' }), + ); expect(output).toEqual({ host: 'gitlab.com', apiBaseUrl: 'https://gitlab.com/api/v4', @@ -88,12 +92,15 @@ describe('readGitLabIntegrationConfigs', () => { { host: 'a.com', token: 't', + apiBaseUrl: 'https://a.com/api/v4', + baseUrl: 'https://a.com', }, ]), ); expect(output).toContainEqual({ host: 'a.com', token: 't', + apiBaseUrl: 'https://a.com/api/v4', baseUrl: 'https://a.com', }); }); @@ -104,6 +111,7 @@ describe('readGitLabIntegrationConfigs', () => { { host: 'gitlab.com', apiBaseUrl: 'https://gitlab.com/api/v4', + baseUrl: 'https://gitlab.com', }, ]); }); diff --git a/packages/integration/src/gitlab/config.ts b/packages/integration/src/gitlab/config.ts index 47269b4c32..cf717e08d2 100644 --- a/packages/integration/src/gitlab/config.ts +++ b/packages/integration/src/gitlab/config.ts @@ -15,7 +15,7 @@ */ import { Config } from '@backstage/config'; -import { isValidHost } from '../helpers'; +import { isValidHost, isValidUrl } from '../helpers'; const GITLAB_HOST = 'gitlab.com'; const GITLAB_API_BASE_URL = 'https://gitlab.com/api/v4'; @@ -38,7 +38,7 @@ export type GitLabIntegrationConfig = { * The API will always be preferred if both its base URL and a token are * present. */ - apiBaseUrl?: string; + apiBaseUrl: string; /** * The authorization token to use for requests this provider. @@ -53,7 +53,7 @@ export type GitLabIntegrationConfig = { * * If no baseUrl is provided, it will default to https://${host} */ - baseUrl?: string; + baseUrl: string; }; /** @@ -64,16 +64,10 @@ export type GitLabIntegrationConfig = { export function readGitLabIntegrationConfig( config: Config, ): GitLabIntegrationConfig { - const host = config.getOptionalString('host') ?? GITLAB_HOST; + const host = config.getString('host'); let apiBaseUrl = config.getOptionalString('apiBaseUrl'); const token = config.getOptionalString('token'); - const baseUrl = config.getOptionalString('baseUrl') ?? `https://${host}`; - - if (!isValidHost(host)) { - throw new Error( - `Invalid GitLab integration config, '${host}' is not a valid host`, - ); - } + let baseUrl = config.getOptionalString('baseUrl'); if (apiBaseUrl) { apiBaseUrl = apiBaseUrl.replace(/\/+$/, ''); @@ -81,6 +75,30 @@ export function readGitLabIntegrationConfig( apiBaseUrl = GITLAB_API_BASE_URL; } + if (baseUrl) { + baseUrl = baseUrl.replace(/\/+$/, ''); + } else { + baseUrl = `https://${host}`; + } + + if (host.includes(':')) { + throw new Error( + `Invalid GitLab integration config, host '${host}' should just be the host name (e.g. "github.com"), not a URL`, + ); + } else if (!isValidHost(host)) { + throw new Error( + `Invalid GitLab integration config, '${host}' is not a valid host`, + ); + } else if (!apiBaseUrl || !isValidUrl(apiBaseUrl)) { + throw new Error( + `Invalid GitLab integration config, '${apiBaseUrl}' is not a valid apiBaseUrl`, + ); + } else if (!isValidUrl(baseUrl)) { + throw new Error( + `Invalid GitLab integration config, '${baseUrl}' is not a valid baseUrl`, + ); + } + return { host, token, apiBaseUrl, baseUrl }; } @@ -99,7 +117,11 @@ export function readGitLabIntegrationConfigs( // As a convenience we always make sure there's at least an unauthenticated // reader for public gitlab repos. if (!result.some(c => c.host === GITLAB_HOST)) { - result.push({ host: GITLAB_HOST, apiBaseUrl: GITLAB_API_BASE_URL }); + result.push({ + host: GITLAB_HOST, + apiBaseUrl: GITLAB_API_BASE_URL, + baseUrl: `https://${GITLAB_HOST}`, + }); } return result; diff --git a/packages/integration/src/gitlab/core.test.ts b/packages/integration/src/gitlab/core.test.ts index 2b4713604c..6836854eae 100644 --- a/packages/integration/src/gitlab/core.test.ts +++ b/packages/integration/src/gitlab/core.test.ts @@ -37,10 +37,14 @@ describe('gitlab core', () => { const configWithToken: GitLabIntegrationConfig = { host: 'g.com', token: '0123456789', + apiBaseUrl: '', + baseUrl: '', }; const configWithNoToken: GitLabIntegrationConfig = { host: 'g.com', + apiBaseUrl: '', + baseUrl: '', }; describe('getGitLabFileFetchUrl', () => { diff --git a/packages/integration/src/helpers.ts b/packages/integration/src/helpers.ts index cc1c59a238..73cef18297 100644 --- a/packages/integration/src/helpers.ts +++ b/packages/integration/src/helpers.ts @@ -16,11 +16,22 @@ import { ScmIntegration, ScmIntegrationsGroup } from './types'; -/** Checks whether the given url is a valid host */ -export function isValidHost(url: string): boolean { +/** Checks whether the given argument is a valid URL hostname */ +export function isValidHost(host: string): boolean { const check = new URL('http://example.com'); - check.host = url; - return check.host === url; + check.host = host; + return check.host === host; +} + +/** Checks whether the given argument is a valid URL */ +export function isValidUrl(url: string): boolean { + try { + // eslint-disable-next-line no-new + new URL(url); + return true; + } catch { + return false; + } } export function basicIntegrations( diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/prepare/gitlab.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/prepare/gitlab.test.ts index 6ed4651e4f..2de2326505 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/prepare/gitlab.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/prepare/gitlab.test.ts @@ -37,8 +37,10 @@ describe('GitLabPreparer', () => { jest.clearAllMocks(); }); const preparer = GitlabPreparer.fromConfig({ - host: 'gitlab.com', + host: '', token: 'fake-token', + apiBaseUrl: '', + baseUrl: '', }); it(`calls the clone command with the correct arguments for a repository`, async () => { diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/gitlab.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/gitlab.test.ts index 61c8480efb..3826a54ddb 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/gitlab.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/gitlab.test.ts @@ -57,6 +57,7 @@ describe('GitLab Publisher', () => { const publisher = await GitlabPublisher.fromConfig( { host: 'gitlab.com', + apiBaseUrl: 'https://gitlab.com/api/v4', token: 'fake-token', baseUrl: 'https://gitlab.hosted.com', }, @@ -105,7 +106,9 @@ describe('GitLab Publisher', () => { const publisher = await GitlabPublisher.fromConfig( { host: 'gitlab.com', + apiBaseUrl: 'https://gitlab.com/api/v4', token: 'fake-token', + baseUrl: 'https://gitlab.com', }, { repoVisibility: 'public' }, ); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/publishers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/publishers.ts index 30201e6dca..f5f7fc6361 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/publishers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/publishers.ts @@ -126,6 +126,8 @@ export class Publishers implements PublisherBuilder { { token: config.getOptionalString('scaffolder.gitlab.token') ?? '', host: integration.config.host, + apiBaseUrl: ``, + baseUrl: `https://${integration.config.host}`, }, { repoVisibility }, ),