integration: update the gitlab config mandatory fields to match reality

2021-02-08 10:52:10 +01:00
parent abd85269fb
commit ffffea8e6d
12 changed files with 111 additions and 36 deletions
@@ -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.
@@ -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.
@@ -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');
-    });
  });
 });
@@ -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<Buffer> {
@@ -26,6 +26,8 @@ describe('GitLabIntegration', () => {
            {
              host: 'h.com',
              token: 't',
+              apiBaseUrl: 'https://h.com/api/v4',
+              baseUrl: 'https://h.com',
            },
          ],
        },
@@ -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',
      },
    ]);
  });
@@ -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;
@@ -37,10 +37,14 @@ describe('gitlab core', () => {
  const configWithToken: GitLabIntegrationConfig = {
    host: 'g.com',
    token: '0123456789',
+    apiBaseUrl: '<ignored>',
+    baseUrl: '<ignored>',
  };

  const configWithNoToken: GitLabIntegrationConfig = {
    host: 'g.com',
+    apiBaseUrl: '<ignored>',
+    baseUrl: '<ignored>',
  };

  describe('getGitLabFileFetchUrl', () => {
@@ -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<T extends ScmIntegration>(
@@ -37,8 +37,10 @@ describe('GitLabPreparer', () => {
    jest.clearAllMocks();
  });
  const preparer = GitlabPreparer.fromConfig({
-    host: 'gitlab.com',
+    host: '<ignored>',
    token: 'fake-token',
+    apiBaseUrl: '<ignored>',
+    baseUrl: '<ignored>',
  });

  it(`calls the clone command with the correct arguments for a repository`, async () => {
@@ -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' },
      );
@@ -126,6 +126,8 @@ export class Publishers implements PublisherBuilder {
            {
              token: config.getOptionalString('scaffolder.gitlab.token') ?? '',
              host: integration.config.host,
+              apiBaseUrl: `<ignored>`,
+              baseUrl: `https://${integration.config.host}`,
            },
            { repoVisibility },
          ),