Repository: paularmstrong/build-tracker (ts)

Issue:
Allow longer branch names
<!--

IMPORTANT!
Follow the format of this template or your issue will be automatically closed
by a bot. Following this template helps keep the quality of bug reports
high, which will help them be addressed sooner.

-->

## Problem

<!--
Add a short explanation of your problem or use-case

Example:
When I click the drawer link, nothing happens!
-->

## Steps to Reproduce

<!--
Include a list of steps that anyone can take to reproduce the bug
If you have a code sample, this is a great place to include it

Example:
1. Navigate to the main Build Tracker page
2. Click the menu button to open the side drawer
3. Click "Documentation" link
-->

New issue because https://github.com/paularmstrong/build-tracker/issues/203 was automatically closed

The database setup defines a branch as max 64 characters. Just today we had a build fail because we actually ran into this limit (not a branch name we defined ourselves, but one created by GitHub's "revert PR" functionality).

Could we increase the max length?

## Expected Result

<!--
Write out what you expect to see or have happen when running the previous steps

Example:
The link should open a new page to the documentation
-->

## Actual Result

<!--
Write out what *actually* happens when running the previous steps

Example:
Nothing happens. But I do see an error in the browser's console. It looks like this:
> TypeError: cannot read property href of undefined
-->


Pull request description:
fix: increase branch name length to 256-char
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

Some people use really long git branch names, sometimes automated systems do that. We currently limit branch names in the official database plugins to 64 characters.

# Solution

Increase to 256 characters.

Fixes #205 

Ideally in a longer term solution, we should include schema versions in the database to know what migrations need to be run. This is something we could investigate as a larger epic.

# TODO

- [x] Manual verification with docker containers: https://buildtracker.dev/docs/guides/contributing#plugins-plugins
- [x] 🤓 Add & update tests (always try to _increase_ test coverage)
- [x] 🔬 Ensure CI is passing (`yarn lint:ci`, `yarn test`, `yarn tsc:ci`)
- [x] 📖 Update relevant documentation


Patch:
diff --git a/plugins/with-mariadb/src/setup.ts b/plugins/with-mariadb/src/setup.ts
index 03b5aec..7851e8c 100644
--- a/plugins/with-mariadb/src/setup.ts
+++ b/plugins/with-mariadb/src/setup.ts
@@ -4,13 +4,13 @@
 import { Pool } from 'mariadb';
 
 export default function setup(pool: Pool): () => Promise<boolean> {
-  const setup = async (): Promise<boolean> => {
+  return async function setup(): Promise<boolean> {
     const client = await pool.getConnection();
     try {
       await client.query(`
 CREATE TABLE IF NOT EXISTS builds(
   revision VARCHAR(64) PRIMARY KEY NOT NULL,
-  branch VARCHAR(64) NOT NULL,
+  branch VARCHAR(256) NOT NULL,
   parentRevision VARCHAR(64),
   timestamp INT NOT NULL,
   meta VARCHAR(1024),
@@ -19,6 +19,7 @@ CREATE TABLE IF NOT EXISTS builds(
 )`);
       await client.query('CREATE INDEX IF NOT EXISTS parent ON builds (revision, parentRevision, branch)');
       await client.query('CREATE INDEX IF NOT EXISTS timestamp ON builds (timestamp)');
+      await client.query('ALTER TABLE builds MODIFY branch VARCHAR(256)');
     } catch (err) {
       client.release();
       throw err;
@@ -26,7 +27,4 @@ CREATE TABLE IF NOT EXISTS builds(
     client.release();
     return Promise.resolve(true);
   };
-  return setup;
 }
-
-module.exports = setup;
diff --git a/plugins/with-mysql/src/setup.ts b/plugins/with-mysql/src/setup.ts
index a675e68..6b0b313 100644
--- a/plugins/with-mysql/src/setup.ts
+++ b/plugins/with-mysql/src/setup.ts
@@ -4,18 +4,20 @@
 import { Pool } from 'mysql';
 
 export default function setup(pool: Pool): () => Promise<boolean> {
-  const setup = (): Promise<boolean> => {
+  return async function setup(): Promise<boolean> {
     return new Promise((resolve) => {
       pool.getConnection((err, client) => {
         if (err) {
-          client.release();
+          if (client) {
+            client.release();
+          }
           throw err;
         }
         client.query(
           `
 CREATE TABLE IF NOT EXISTS builds(
   revision VARCHAR(64) PRIMARY KEY NOT NULL,
-  branch VARCHAR(64) NOT NULL,
+  branch VARCHAR(256) NOT NULL,
   parentRevision VARCHAR(64),
   timestamp INT NOT NULL,
   meta VARCHAR(1024),
@@ -37,8 +39,14 @@ CREATE TABLE IF NOT EXISTS builds(
                   client.release();
                   throw err;
                 }
-                client.release();
-                resolve(true);
+                client.query('ALTER TABLE builds MODIFY branch VARCHAR(256)', (err) => {
+                  if (err) {
+                    client.release();
+                    throw err;
+                  }
+                  client.release();
+                  resolve(true);
+                });
               });
             });
           }
@@ -46,8 +54,4 @@ CREATE TABLE IF NOT EXISTS builds(
       });
     });
   };
-
-  return setup;
 }
-
-module.exports = setup;
diff --git a/plugins/with-postgres/src/setup.ts b/plugins/with-postgres/src/setup.ts
index c691419..7e0fe77 100644
--- a/plugins/with-postgres/src/setup.ts
+++ b/plugins/with-postgres/src/setup.ts
@@ -4,12 +4,12 @@
 import { Pool } from 'pg';
 
 export default function setup(pool: Pool): () => Promise<boolean> {
-  const setup = async (): Promise<boolean> => {
+  return async function setup(): Promise<boolean> {
     const client = await pool.connect();
     try {
       await client.query(`CREATE TABLE IF NOT EXISTS builds(
   revision char(64) PRIMARY KEY NOT NULL,
-  branch char(64) NOT NULL,
+  branch char(256) NOT NULL,
   parentRevision char(64),
   timestamp int NOT NULL,
   meta jsonb NOT NULL,
@@ -17,6 +17,7 @@ export default function setup(pool: Pool): () => Promise<boolean> {
 )`);
       await client.query('CREATE INDEX IF NOT EXISTS parent ON builds (revision, parentRevision, branch)');
       await client.query('CREATE INDEX IF NOT EXISTS timestamp ON builds (timestamp)');
+      await client.query('ALTER TABLE builds ALTER COLUMN branch TYPE VARCHAR(256)');
     } catch (err) {
       client.release();
       throw err;
@@ -24,7 +25,4 @@ export default function setup(pool: Pool): () => Promise<boolean> {
     client.release();
     return Promise.resolve(true);
   };
-  return setup;
 }
-
-module.exports = setup;
diff --git a/src/server/src/commands/seed.ts b/src/server/src/commands/seed.ts
index fa0964a..db7471d 100644
--- a/src/server/src/commands/seed.ts
+++ b/src/server/src/commands/seed.ts
@@ -31,10 +31,12 @@ export const handler = async (args: Args): Promise<void> => {
 
   await config.setup();
 
-  glob.sync(`${path.join(fixturePath, 'builds')}/*.json`).forEach(async (build) => {
+  const builds = glob.sync(`${path.join(fixturePath, 'builds-medium')}/*.json`);
+
+  for (const build of builds) {
     const buildData = require(build);
     await config.queries.build.insert(new Build(buildData.meta, buildData.artifacts));
-  });
+  }
 
   return Promise.resolve();
 };


Test patch:
diff --git a/plugins/with-mariadb/src/__tests__/setup.test.ts b/plugins/with-mariadb/src/__tests__/setup.test.ts
index bbb1678..db39295 100644
--- a/plugins/with-mariadb/src/__tests__/setup.test.ts
+++ b/plugins/with-mariadb/src/__tests__/setup.test.ts
@@ -35,6 +35,11 @@ describe('withMariadb setup', () => {
     expect(query).toHaveBeenCalledWith('CREATE INDEX IF NOT EXISTS timestamp ON builds (timestamp)');
   });
 
+  test('alters the branch column to length 256', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith('ALTER TABLE builds MODIFY branch VARCHAR(256)');
+  });
+
   test('releases the client on complete', async () => {
     await expect(setupFn()).resolves.toBe(true);
     expect(release).toHaveBeenCalled();
diff --git a/plugins/with-mysql/src/__tests__/setup.test.ts b/plugins/with-mysql/src/__tests__/setup.test.ts
index 60586a0..2e3ed2d 100644
--- a/plugins/with-mysql/src/__tests__/setup.test.ts
+++ b/plugins/with-mysql/src/__tests__/setup.test.ts
@@ -43,6 +43,11 @@ describe('withMysql setup', () => {
     expect(query).toHaveBeenCalledWith('ALTER TABLE builds ADD INDEX (timestamp)', expect.any(Function));
   });
 
+  test('alters the branch column to length 256', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith('ALTER TABLE builds MODIFY branch VARCHAR(256)', expect.any(Function));
+  });
+
   test('releases the client on complete', async () => {
     await expect(setupFn()).resolves.toBe(true);
     expect(release).toHaveBeenCalled();
diff --git a/plugins/with-postgres/src/__tests__/setup.test.ts b/plugins/with-postgres/src/__tests__/setup.test.ts
index c5fb2ff..efcfb47 100644
--- a/plugins/with-postgres/src/__tests__/setup.test.ts
+++ b/plugins/with-postgres/src/__tests__/setup.test.ts
@@ -19,39 +19,37 @@ describe('withPostgres', () => {
     setupFn = setup(new Pool());
   });
 
-  test('creates the table if not exists', () => {
-    return setupFn().then((result) => {
-      expect(result).toBe(true);
-      expect(query).toHaveBeenCalledWith(expect.stringMatching('CREATE TABLE IF NOT EXISTS builds'));
-    });
+  test('creates the table if not exists', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith(expect.stringMatching('CREATE TABLE IF NOT EXISTS builds'));
   });
 
-  test('creates a multi-index', () => {
-    return setupFn().then(() => {
-      expect(query).toHaveBeenCalledWith(
-        'CREATE INDEX IF NOT EXISTS parent ON builds (revision, parentRevision, branch)'
-      );
-    });
+  test('creates a multi-index', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith(
+      'CREATE INDEX IF NOT EXISTS parent ON builds (revision, parentRevision, branch)'
+    );
   });
 
-  test('creates an index on timestamp', () => {
-    return setupFn().then(() => {
-      expect(query).toHaveBeenCalledWith('CREATE INDEX IF NOT EXISTS timestamp ON builds (timestamp)');
-    });
+  test('creates an index on timestamp', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith('CREATE INDEX IF NOT EXISTS timestamp ON builds (timestamp)');
   });
 
-  test('releases the client on complete', () => {
-    return setupFn().then(() => {
-      expect(release).toHaveBeenCalled();
-    });
+  test('alters the branch column to length 256', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(query).toHaveBeenCalledWith('ALTER TABLE builds ALTER COLUMN branch TYPE VARCHAR(256)');
   });
 
-  test('releases the client on error', () => {
+  test('releases the client on complete', async () => {
+    await expect(setupFn()).resolves.toBe(true);
+    expect(release).toHaveBeenCalled();
+  });
+
+  test('releases the client on error', async () => {
     const error = new Error('tacos');
     query.mockReturnValueOnce(Promise.reject(error));
-    return setupFn().catch((err) => {
-      expect(release).toHaveBeenCalled();
-      expect(err).toBe(error);
-    });
+    await expect(setupFn()).rejects.toEqual(error);
+    expect(release).toHaveBeenCalled();
   });
 });