From 65c2039d0c63ccc7a98cdfd6c9ff18c542e1035e Mon Sep 17 00:00:00 2001 From: Hector Valcarcel Date: Fri, 7 Jun 2024 17:57:54 +0200 Subject: [PATCH 01/20] Chore: Allow using an AWS_SESSION_TOKEN for object storage with AWS S3 --- packages/backend-core/src/environment.ts | 1 + packages/backend-core/src/objectStore/objectStore.ts | 5 +++++ packages/server/src/environment.ts | 1 + packages/worker/src/environment.ts | 1 + 4 files changed, 8 insertions(+) diff --git a/packages/backend-core/src/environment.ts b/packages/backend-core/src/environment.ts index 1e7da2f9a2..0c7f84220b 100644 --- a/packages/backend-core/src/environment.ts +++ b/packages/backend-core/src/environment.ts @@ -120,6 +120,7 @@ const environment = { REDIS_CLUSTERED: process.env.REDIS_CLUSTERED, MINIO_ACCESS_KEY: process.env.MINIO_ACCESS_KEY, MINIO_SECRET_KEY: process.env.MINIO_SECRET_KEY, + AWS_SESSION_TOKEN: process.env.AWS_SESSION_TOKEN, AWS_REGION: process.env.AWS_REGION, MINIO_URL: process.env.MINIO_URL, MINIO_ENABLED: process.env.MINIO_ENABLED || 1, diff --git a/packages/backend-core/src/objectStore/objectStore.ts b/packages/backend-core/src/objectStore/objectStore.ts index de94e3968b..68b1b10ec2 100644 --- a/packages/backend-core/src/objectStore/objectStore.ts +++ b/packages/backend-core/src/objectStore/objectStore.ts @@ -101,6 +101,11 @@ export function ObjectStore( } } + // for AWS Credentials using temporary session token + if (!env.MINIO_ENABLED && env.AWS_SESSION_TOKEN) { + config.sessionToken = env.AWS_SESSION_TOKEN + } + // custom S3 is in use i.e. minio if (env.MINIO_URL) { if (opts.presigning && env.MINIO_ENABLED) { diff --git a/packages/server/src/environment.ts b/packages/server/src/environment.ts index b44d7547a2..341483d861 100644 --- a/packages/server/src/environment.ts +++ b/packages/server/src/environment.ts @@ -48,6 +48,7 @@ const environment = { MINIO_URL: process.env.MINIO_URL, WORKER_URL: process.env.WORKER_URL, AWS_REGION: process.env.AWS_REGION, + AWS_SESSION_TOKEN: process.env.AWS_SESSION_TOKEN, MINIO_ACCESS_KEY: process.env.MINIO_ACCESS_KEY, MINIO_SECRET_KEY: process.env.MINIO_SECRET_KEY, REDIS_URL: process.env.REDIS_URL, diff --git a/packages/worker/src/environment.ts b/packages/worker/src/environment.ts index 70fb911ee1..f6b3701be1 100644 --- a/packages/worker/src/environment.ts +++ b/packages/worker/src/environment.ts @@ -24,6 +24,7 @@ const environment = { // auth MINIO_ACCESS_KEY: process.env.MINIO_ACCESS_KEY, MINIO_SECRET_KEY: process.env.MINIO_SECRET_KEY, + AWS_SESSION_TOKEN: process.env.AWS_SESSION_TOKEN, SALT_ROUNDS: process.env.SALT_ROUNDS, REDIS_PASSWORD: process.env.REDIS_PASSWORD, COOKIE_DOMAIN: process.env.COOKIE_DOMAIN, From 8f6cfb76341fe41af6db30e2932998bfc0af992b Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 14:30:11 +0100 Subject: [PATCH 02/20] Disabling app migration tests and app migrations in test environment. --- .../migrations/tests/20240604153647_initial_sqs.spec.ts | 2 +- .../server/src/appMigrations/tests/migrations.integrity.spec.ts | 2 +- packages/server/src/appMigrations/tests/migrations.spec.ts | 2 +- .../server/src/appMigrations/tests/migrationsProcessor.spec.ts | 2 +- packages/server/src/middleware/appMigrations.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index 86e50a5812..bb31410882 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -82,7 +82,7 @@ beforeAll(async () => { }) }) -describe("SQS migration", () => { +xdescribe("SQS migration", () => { it("test migration runs as expected against an older DB", async () => { const db = dbCore.getDB(config.appId!) // confirm nothing exists initially diff --git a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts index 145a06d7f5..88572f23fa 100644 --- a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts @@ -2,7 +2,7 @@ import { context } from "@budibase/backend-core" import * as setup from "../../api/routes/tests/utilities" import * as migrations from "../migrations" -describe("migration integrity", () => { +xdescribe("migration integrity", () => { // These test is checking that each migration is "idempotent". // We should be able to rerun any migration, with any rerun not modifiying anything. The code should be aware that the migration already ran it("each migration can rerun safely", async () => { diff --git a/packages/server/src/appMigrations/tests/migrations.spec.ts b/packages/server/src/appMigrations/tests/migrations.spec.ts index 1da94f503f..c644810436 100644 --- a/packages/server/src/appMigrations/tests/migrations.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.spec.ts @@ -13,7 +13,7 @@ jest.mock("../migrations", () => ({ ], })) -describe("migrations", () => { +xdescribe("migrations", () => { it("new apps are created with the latest app migration version set", async () => { const config = setup.getConfig() await config.init() diff --git a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts index a2d1dc05c3..eb2ee5a03d 100644 --- a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts +++ b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts @@ -6,7 +6,7 @@ import { AppMigration } from ".." const futureTimestamp = `20500101174029` -describe("migrationsProcessor", () => { +xdescribe("migrationsProcessor", () => { it("running migrations will update the latest applied migration", async () => { const testMigrations: AppMigration[] = [ { id: `${futureTimestamp}_123`, func: async () => {} }, diff --git a/packages/server/src/middleware/appMigrations.ts b/packages/server/src/middleware/appMigrations.ts index 6ad356427b..9c739fb72b 100644 --- a/packages/server/src/middleware/appMigrations.ts +++ b/packages/server/src/middleware/appMigrations.ts @@ -7,7 +7,7 @@ export default async (ctx: UserCtx, next: any) => { // migrations can be disabled via environment variable if you // need to completely disable migrations, e.g. for testing - if (env.DISABLE_APP_MIGRATIONS) { + if (env.DISABLE_APP_MIGRATIONS || env.isTest()) { return next() } From 1c1074d93964016a77e6bdd44977d748a039c28f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 14:48:39 +0100 Subject: [PATCH 03/20] Updating to describe.skip --- .../migrations/tests/20240604153647_initial_sqs.spec.ts | 2 +- .../server/src/appMigrations/tests/migrations.integrity.spec.ts | 2 +- packages/server/src/appMigrations/tests/migrations.spec.ts | 2 +- .../server/src/appMigrations/tests/migrationsProcessor.spec.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index bb31410882..731f0364c3 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -82,7 +82,7 @@ beforeAll(async () => { }) }) -xdescribe("SQS migration", () => { +describe.skip("SQS migration", () => { it("test migration runs as expected against an older DB", async () => { const db = dbCore.getDB(config.appId!) // confirm nothing exists initially diff --git a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts index 88572f23fa..1a7cb70711 100644 --- a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts @@ -2,7 +2,7 @@ import { context } from "@budibase/backend-core" import * as setup from "../../api/routes/tests/utilities" import * as migrations from "../migrations" -xdescribe("migration integrity", () => { +describe.skip("migration integrity", () => { // These test is checking that each migration is "idempotent". // We should be able to rerun any migration, with any rerun not modifiying anything. The code should be aware that the migration already ran it("each migration can rerun safely", async () => { diff --git a/packages/server/src/appMigrations/tests/migrations.spec.ts b/packages/server/src/appMigrations/tests/migrations.spec.ts index c644810436..e707c9914c 100644 --- a/packages/server/src/appMigrations/tests/migrations.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.spec.ts @@ -13,7 +13,7 @@ jest.mock("../migrations", () => ({ ], })) -xdescribe("migrations", () => { +describe.skip("migrations", () => { it("new apps are created with the latest app migration version set", async () => { const config = setup.getConfig() await config.init() diff --git a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts index eb2ee5a03d..b552e2bb12 100644 --- a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts +++ b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts @@ -6,7 +6,7 @@ import { AppMigration } from ".." const futureTimestamp = `20500101174029` -xdescribe("migrationsProcessor", () => { +describe.skip("migrationsProcessor", () => { it("running migrations will update the latest applied migration", async () => { const testMigrations: AppMigration[] = [ { id: `${futureTimestamp}_123`, func: async () => {} }, From 04297e8f3659749788f0c48d6255577ce8cafe7e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 15:15:35 +0100 Subject: [PATCH 04/20] skip to re-run. --- .../server/src/appMigrations/tests/migrations.integrity.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts index 1a7cb70711..2bb4c2b4f8 100644 --- a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts @@ -5,7 +5,7 @@ import * as migrations from "../migrations" describe.skip("migration integrity", () => { // These test is checking that each migration is "idempotent". // We should be able to rerun any migration, with any rerun not modifiying anything. The code should be aware that the migration already ran - it("each migration can rerun safely", async () => { + it.skip("each migration can rerun safely", async () => { const config = setup.getConfig() await config.init() From fe789e7462989ac39364d0aa80e8174eab7969e6 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 15:17:01 +0100 Subject: [PATCH 05/20] Adding test containers debug. --- packages/backend-core/tests/jestEnv.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend-core/tests/jestEnv.ts b/packages/backend-core/tests/jestEnv.ts index 2c797c9fff..10bd1ade39 100644 --- a/packages/backend-core/tests/jestEnv.ts +++ b/packages/backend-core/tests/jestEnv.ts @@ -8,3 +8,4 @@ process.env.COUCH_DB_PASSWORD = "budibase" process.env.COUCH_DB_USER = "budibase" process.env.API_ENCRYPTION_KEY = "testsecret" process.env.JWT_SECRET = "testsecret" +process.env.DEBUG = "testcontainers*" From 1bd5a41712de5f3ffc3334ed9643295e36abfd00 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 15:29:32 +0100 Subject: [PATCH 06/20] Setting debug properly --- .github/workflows/budibase_ci.yml | 1 + packages/backend-core/tests/jestEnv.ts | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index 7f1e08601a..cb8a2bb4d6 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -27,6 +27,7 @@ env: NX_BASE_BRANCH: origin/${{ github.base_ref }} USE_NX_AFFECTED: ${{ github.event_name == 'pull_request' }} IS_OSS_CONTRIBUTOR: ${{ inputs.run_as_oss == true || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != 'Budibase/budibase') }} + DEBUG: testcontainers* jobs: lint: diff --git a/packages/backend-core/tests/jestEnv.ts b/packages/backend-core/tests/jestEnv.ts index 10bd1ade39..2c797c9fff 100644 --- a/packages/backend-core/tests/jestEnv.ts +++ b/packages/backend-core/tests/jestEnv.ts @@ -8,4 +8,3 @@ process.env.COUCH_DB_PASSWORD = "budibase" process.env.COUCH_DB_USER = "budibase" process.env.API_ENCRYPTION_KEY = "testsecret" process.env.JWT_SECRET = "testsecret" -process.env.DEBUG = "testcontainers*" From 8665737dcbeb1dbe6f81873385afffa533c6ae3a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 17:18:27 +0100 Subject: [PATCH 07/20] Bringing app migration tests back. --- .../migrations/tests/20240604153647_initial_sqs.spec.ts | 2 +- .../src/appMigrations/tests/migrations.integrity.spec.ts | 4 ++-- packages/server/src/appMigrations/tests/migrations.spec.ts | 2 +- .../src/appMigrations/tests/migrationsProcessor.spec.ts | 2 +- packages/server/src/middleware/appMigrations.ts | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index 731f0364c3..86e50a5812 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -82,7 +82,7 @@ beforeAll(async () => { }) }) -describe.skip("SQS migration", () => { +describe("SQS migration", () => { it("test migration runs as expected against an older DB", async () => { const db = dbCore.getDB(config.appId!) // confirm nothing exists initially diff --git a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts index 2bb4c2b4f8..145a06d7f5 100644 --- a/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.integrity.spec.ts @@ -2,10 +2,10 @@ import { context } from "@budibase/backend-core" import * as setup from "../../api/routes/tests/utilities" import * as migrations from "../migrations" -describe.skip("migration integrity", () => { +describe("migration integrity", () => { // These test is checking that each migration is "idempotent". // We should be able to rerun any migration, with any rerun not modifiying anything. The code should be aware that the migration already ran - it.skip("each migration can rerun safely", async () => { + it("each migration can rerun safely", async () => { const config = setup.getConfig() await config.init() diff --git a/packages/server/src/appMigrations/tests/migrations.spec.ts b/packages/server/src/appMigrations/tests/migrations.spec.ts index e707c9914c..1da94f503f 100644 --- a/packages/server/src/appMigrations/tests/migrations.spec.ts +++ b/packages/server/src/appMigrations/tests/migrations.spec.ts @@ -13,7 +13,7 @@ jest.mock("../migrations", () => ({ ], })) -describe.skip("migrations", () => { +describe("migrations", () => { it("new apps are created with the latest app migration version set", async () => { const config = setup.getConfig() await config.init() diff --git a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts index b552e2bb12..a2d1dc05c3 100644 --- a/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts +++ b/packages/server/src/appMigrations/tests/migrationsProcessor.spec.ts @@ -6,7 +6,7 @@ import { AppMigration } from ".." const futureTimestamp = `20500101174029` -describe.skip("migrationsProcessor", () => { +describe("migrationsProcessor", () => { it("running migrations will update the latest applied migration", async () => { const testMigrations: AppMigration[] = [ { id: `${futureTimestamp}_123`, func: async () => {} }, diff --git a/packages/server/src/middleware/appMigrations.ts b/packages/server/src/middleware/appMigrations.ts index 9c739fb72b..6ad356427b 100644 --- a/packages/server/src/middleware/appMigrations.ts +++ b/packages/server/src/middleware/appMigrations.ts @@ -7,7 +7,7 @@ export default async (ctx: UserCtx, next: any) => { // migrations can be disabled via environment variable if you // need to completely disable migrations, e.g. for testing - if (env.DISABLE_APP_MIGRATIONS || env.isTest()) { + if (env.DISABLE_APP_MIGRATIONS) { return next() } From c00e9ef9466d92ca7a511de9e45dffc8563185c4 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 17:52:44 +0100 Subject: [PATCH 08/20] Check disk space. --- .github/workflows/budibase_ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index e54bb412b4..4b3947eb19 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -179,6 +179,8 @@ jobs: - run: yarn --frozen-lockfile + - run: df -h + - name: Test server env: DD_CIVISIBILITY_AGENTLESS_ENABLED: true From 0c9881706de656abe6d8e4cfa404781d47f069ba Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 10 Jun 2024 21:56:00 +0100 Subject: [PATCH 09/20] Removing debug. --- .github/workflows/budibase_ci.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index 4b3947eb19..eb11627758 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -27,7 +27,6 @@ env: NX_BASE_BRANCH: origin/${{ github.base_ref }} USE_NX_AFFECTED: ${{ github.event_name == 'pull_request' }} IS_OSS_CONTRIBUTOR: ${{ inputs.run_as_oss == true || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != 'Budibase/budibase') }} - DEBUG: testcontainers* jobs: lint: @@ -179,8 +178,6 @@ jobs: - run: yarn --frozen-lockfile - - run: df -h - - name: Test server env: DD_CIVISIBILITY_AGENTLESS_ENABLED: true From 96e0636af618988c5cb6248bf32f3ae918aae94a Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 09:56:10 +0100 Subject: [PATCH 10/20] Allow SSHing into server tests. --- .github/workflows/budibase_ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index eb11627758..ad398a9bda 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -156,6 +156,8 @@ jobs: token: ${{ secrets.PERSONAL_ACCESS_TOKEN || github.token }} fetch-depth: 0 + - uses: valeriangalliat/action-sshd-cloudflared@v2 + - name: Use Node.js 20.x uses: actions/setup-node@v4 with: From c82f7c209deb1e7174b1fa06e09208eca8c314ad Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 10:08:49 +0100 Subject: [PATCH 11/20] Move SSH connection down a bit. --- .github/workflows/budibase_ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index ad398a9bda..cdff56f91c 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -156,8 +156,6 @@ jobs: token: ${{ secrets.PERSONAL_ACCESS_TOKEN || github.token }} fetch-depth: 0 - - uses: valeriangalliat/action-sshd-cloudflared@v2 - - name: Use Node.js 20.x uses: actions/setup-node@v4 with: @@ -180,6 +178,8 @@ jobs: - run: yarn --frozen-lockfile + - uses: valeriangalliat/action-sshd-cloudflared@v2 + - name: Test server env: DD_CIVISIBILITY_AGENTLESS_ENABLED: true From 1d1ca694c83f8da2ed709f0de3c4d70cad710564 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 14:39:44 +0100 Subject: [PATCH 12/20] Lock starting containers. --- .gitignore | 2 + .../src/integrations/tests/utils/index.ts | 53 ++++++++++++++----- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index b68ddd975f..32d1416f4a 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,8 @@ bb-airgapped.tar.gz packages/server/build/oldClientVersions/**/* packages/builder/src/components/deploy/clientVersions.json +packages/server/src/integrations/tests/utils/*.lock + # Logs logs *.log diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index a54d0ac1a7..64e466e6a0 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -6,6 +6,9 @@ import * as mssql from "./mssql" import * as mariadb from "./mariadb" import { GenericContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" +import lockfile from "proper-lockfile" +import path from "path" +import fs from "fs" export type DatasourceProvider = () => Promise @@ -67,20 +70,44 @@ export async function rawQuery(ds: Datasource, sql: string): Promise { export async function startContainer(container: GenericContainer) { container = container.withReuse().withLabels({ "com.budibase": "true" }) - const startedContainer = await container.start() + // If two tests try to spin up the same container at the same time, there's a + // possibility that two containers of the same type will be started. To avoid + // this, we use a filesystem lock to ensure that only one container of a given + // type is started at a time. + const imageName = (container as any).imageName.string as string + const lockPath = path.resolve( + __dirname, + `${imageName.replaceAll("/", "-")}.lock` + ) - const info = testContainerUtils.getContainerById(startedContainer.getId()) - if (!info) { - throw new Error("Container not found") + // The `proper-lockfile` library needs the file we're locking on to exist + // before it can lock it. + if (!fs.existsSync(lockPath)) { + fs.writeFileSync(lockPath, "") } - // Some Docker runtimes, when you expose a port, will bind it to both - // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 - // addresses are not shared, and testcontainers will sometimes give you back - // the ipv6 port. There's no way to know that this has happened, and if you - // try to then connect to `localhost:port` you may attempt to bind to the v4 - // address which could be unbound or even an entirely different container. For - // that reason, we don't use testcontainers' `getExposedPort` function, - // preferring instead our own method that guaranteed v4 ports. - return testContainerUtils.getExposedV4Ports(info) + await lockfile.lock(lockPath, { + retries: 10, + }) + + try { + const startedContainer = await container.start() + + const info = testContainerUtils.getContainerById(startedContainer.getId()) + if (!info) { + throw new Error("Container not found") + } + + // Some Docker runtimes, when you expose a port, will bind it to both + // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 + // addresses are not shared, and testcontainers will sometimes give you back + // the ipv6 port. There's no way to know that this has happened, and if you + // try to then connect to `localhost:port` you may attempt to bind to the v4 + // address which could be unbound or even an entirely different container. For + // that reason, we don't use testcontainers' `getExposedPort` function, + // preferring instead our own method that guaranteed v4 ports. + return testContainerUtils.getExposedV4Ports(info) + } finally { + await lockfile.unlock(lockPath) + } } From af0802df303b22a277bd3ed070399e0684ad1bf6 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 14:43:45 +0100 Subject: [PATCH 13/20] Only hold the lock during container start. --- .../src/integrations/tests/utils/index.ts | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 64e466e6a0..537e523fc6 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -4,7 +4,7 @@ import * as mongodb from "./mongodb" import * as mysql from "./mysql" import * as mssql from "./mssql" import * as mariadb from "./mariadb" -import { GenericContainer } from "testcontainers" +import { GenericContainer, StartedTestContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" import lockfile from "proper-lockfile" import path from "path" @@ -90,24 +90,25 @@ export async function startContainer(container: GenericContainer) { retries: 10, }) + let startedContainer: StartedTestContainer try { - const startedContainer = await container.start() - - const info = testContainerUtils.getContainerById(startedContainer.getId()) - if (!info) { - throw new Error("Container not found") - } - - // Some Docker runtimes, when you expose a port, will bind it to both - // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 - // addresses are not shared, and testcontainers will sometimes give you back - // the ipv6 port. There's no way to know that this has happened, and if you - // try to then connect to `localhost:port` you may attempt to bind to the v4 - // address which could be unbound or even an entirely different container. For - // that reason, we don't use testcontainers' `getExposedPort` function, - // preferring instead our own method that guaranteed v4 ports. - return testContainerUtils.getExposedV4Ports(info) + startedContainer = await container.start() } finally { await lockfile.unlock(lockPath) } + + const info = testContainerUtils.getContainerById(startedContainer.getId()) + if (!info) { + throw new Error("Container not found") + } + + // Some Docker runtimes, when you expose a port, will bind it to both + // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 + // addresses are not shared, and testcontainers will sometimes give you back + // the ipv6 port. There's no way to know that this has happened, and if you + // try to then connect to `localhost:port` you may attempt to bind to the v4 + // address which could be unbound or even an entirely different container. For + // that reason, we don't use testcontainers' `getExposedPort` function, + // preferring instead our own method that guaranteed v4 ports. + return testContainerUtils.getExposedV4Ports(info) } From 2e67ae115e024f74d9872921d88787370e43274d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:33:18 +0100 Subject: [PATCH 14/20] Attempt to use unluck we get back from lock. --- packages/server/src/integrations/tests/utils/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 537e523fc6..eaebb8f2d5 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,7 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - await lockfile.lock(lockPath, { + const unlock = await lockfile.lock(lockPath, { retries: 10, }) @@ -94,7 +94,7 @@ export async function startContainer(container: GenericContainer) { try { startedContainer = await container.start() } finally { - await lockfile.unlock(lockPath) + await unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From eac6106b06aced3114ebc42287dbe459ce452357 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:38:25 +0100 Subject: [PATCH 15/20] Try the sync versions? --- packages/server/src/integrations/tests/utils/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index eaebb8f2d5..7487ee079c 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,7 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - const unlock = await lockfile.lock(lockPath, { + const unlock = lockfile.lockSync(lockPath, { retries: 10, }) @@ -94,7 +94,7 @@ export async function startContainer(container: GenericContainer) { try { startedContainer = await container.start() } finally { - await unlock() + unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From af60ff4da77d61ad732dbbc4983b0e8cad0dacf0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:43:12 +0100 Subject: [PATCH 16/20] Can't use retries with the sync API. --- packages/server/src/integrations/tests/utils/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 7487ee079c..a462cdce05 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,9 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - const unlock = lockfile.lockSync(lockPath, { - retries: 10, - }) + const unlock = lockfile.lockSync(lockPath) let startedContainer: StartedTestContainer try { From 6a54b58303002bc42cc62df7511631172554b37b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 16:00:04 +0100 Subject: [PATCH 17/20] ? --- packages/server/specs/openapi.json | 12 +++++++++--- packages/server/specs/openapi.yaml | 6 ++++++ .../server/src/integrations/tests/utils/index.ts | 13 +++++-------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/server/specs/openapi.json b/packages/server/specs/openapi.json index 7d07b424f0..b21554505b 100644 --- a/packages/server/specs/openapi.json +++ b/packages/server/specs/openapi.json @@ -860,8 +860,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, @@ -1067,8 +1069,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, @@ -1285,8 +1289,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, diff --git a/packages/server/specs/openapi.yaml b/packages/server/specs/openapi.yaml index 3a798c424b..6a2ae89c61 100644 --- a/packages/server/specs/openapi.yaml +++ b/packages/server/specs/openapi.yaml @@ -782,8 +782,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: @@ -948,8 +950,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: @@ -1121,8 +1125,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index a462cdce05..d9a2960848 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -80,19 +80,16 @@ export async function startContainer(container: GenericContainer) { `${imageName.replaceAll("/", "-")}.lock` ) - // The `proper-lockfile` library needs the file we're locking on to exist - // before it can lock it. - if (!fs.existsSync(lockPath)) { - fs.writeFileSync(lockPath, "") - } - - const unlock = lockfile.lockSync(lockPath) + const unlock = await lockfile.lock(lockPath, { + retries: 10, + realpath: false, + }) let startedContainer: StartedTestContainer try { startedContainer = await container.start() } finally { - unlock() + await unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From 85c59c0350f87526566e4a071411e8c3d955b29c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 17:41:48 +0100 Subject: [PATCH 18/20] Changing tactic to relying on stable container names to prevent duplication. --- .../src/integrations/tests/utils/index.ts | 49 ++++++++++++------- .../src/integrations/tests/utils/mssql.ts | 3 ++ .../src/integrations/tests/utils/mysql.ts | 3 ++ .../src/integrations/tests/utils/postgres.ts | 3 ++ 4 files changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index d9a2960848..afbf4310a8 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -9,6 +9,7 @@ import { testContainerUtils } from "@budibase/backend-core/tests" import lockfile from "proper-lockfile" import path from "path" import fs from "fs" +import _ from "lodash" export type DatasourceProvider = () => Promise @@ -68,28 +69,38 @@ export async function rawQuery(ds: Datasource, sql: string): Promise { } export async function startContainer(container: GenericContainer) { - container = container.withReuse().withLabels({ "com.budibase": "true" }) - - // If two tests try to spin up the same container at the same time, there's a - // possibility that two containers of the same type will be started. To avoid - // this, we use a filesystem lock to ensure that only one container of a given - // type is started at a time. const imageName = (container as any).imageName.string as string - const lockPath = path.resolve( - __dirname, - `${imageName.replaceAll("/", "-")}.lock` - ) + const key = imageName.replaceAll("/", "-").replaceAll(":", "-") - const unlock = await lockfile.lock(lockPath, { - retries: 10, - realpath: false, - }) + container = container + .withReuse() + .withLabels({ "com.budibase": "true" }) + .withName(key) - let startedContainer: StartedTestContainer - try { - startedContainer = await container.start() - } finally { - await unlock() + let startedContainer: StartedTestContainer | undefined = undefined + let lastError = undefined + for (let i = 0; i < 10; i++) { + try { + // container.start() is not an idempotent operation, calling `start` + // modifies the internal state of a GenericContainer instance such that + // the hash it uses to determine reuse changes. We need to clone the + // container before calling start to ensure that we're using the same + // reuse hash every time. + const containerCopy = _.cloneDeep(container) + startedContainer = await containerCopy.start() + lastError = undefined + break + } catch (e: any) { + lastError = e + await new Promise(resolve => setTimeout(resolve, 1000)) + } + } + + if (!startedContainer) { + if (lastError) { + throw lastError + } + throw new Error(`failed to start container: ${imageName}`) } const info = testContainerUtils.getContainerById(startedContainer.getId()) diff --git a/packages/server/src/integrations/tests/utils/mssql.ts b/packages/server/src/integrations/tests/utils/mssql.ts index 647f461272..57c5fe8049 100644 --- a/packages/server/src/integrations/tests/utils/mssql.ts +++ b/packages/server/src/integrations/tests/utils/mssql.ts @@ -29,6 +29,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 1433)?.host + if (!port) { + throw new Error("SQL Server port not found") + } const datasource: Datasource = { type: "datasource_plus", diff --git a/packages/server/src/integrations/tests/utils/mysql.ts b/packages/server/src/integrations/tests/utils/mysql.ts index a78833e1de..560d6bb2d4 100644 --- a/packages/server/src/integrations/tests/utils/mysql.ts +++ b/packages/server/src/integrations/tests/utils/mysql.ts @@ -38,6 +38,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 3306)?.host + if (!port) { + throw new Error("MySQL port not found") + } const datasource: Datasource = { type: "datasource_plus", diff --git a/packages/server/src/integrations/tests/utils/postgres.ts b/packages/server/src/integrations/tests/utils/postgres.ts index 4191b107e9..8c0cd886e8 100644 --- a/packages/server/src/integrations/tests/utils/postgres.ts +++ b/packages/server/src/integrations/tests/utils/postgres.ts @@ -21,6 +21,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 5432)?.host + if (!port) { + throw new Error("Postgres port not found") + } const datasource: Datasource = { type: "datasource_plus", From 96efb17678e9503f14dfdb98e7e17f060c838b29 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 17:52:02 +0100 Subject: [PATCH 19/20] Fix lint. --- packages/server/src/integrations/tests/utils/index.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index afbf4310a8..64617461bb 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -6,10 +6,7 @@ import * as mssql from "./mssql" import * as mariadb from "./mariadb" import { GenericContainer, StartedTestContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" -import lockfile from "proper-lockfile" -import path from "path" -import fs from "fs" -import _ from "lodash" +import cloneDeep from "lodash/cloneDeep" export type DatasourceProvider = () => Promise @@ -86,7 +83,7 @@ export async function startContainer(container: GenericContainer) { // the hash it uses to determine reuse changes. We need to clone the // container before calling start to ensure that we're using the same // reuse hash every time. - const containerCopy = _.cloneDeep(container) + const containerCopy = cloneDeep(container) startedContainer = await containerCopy.start() lastError = undefined break From 87cf37a4d17427b7a1159b82b3f6ef85f5ebec89 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 18:04:28 +0100 Subject: [PATCH 20/20] Remove SSH in CI. --- .github/workflows/budibase_ci.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index cdff56f91c..eb11627758 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -178,8 +178,6 @@ jobs: - run: yarn --frozen-lockfile - - uses: valeriangalliat/action-sshd-cloudflared@v2 - - name: Test server env: DD_CIVISIBILITY_AGENTLESS_ENABLED: true