Repository: conbench/conbench (language: python)

Issue:
ci: rely only on GitHub Actions for per-PR CI (remove Buildkite pipeline from PRs)
@dianaclarke Were GHA tests enabled because Buildkite pipelines are not public?
ci: rely only on GitHub Actions for per-PR CI (remove Buildkite pipeline from PRs)
@dianaclarke Were GHA tests enabled because Buildkite pipelines are not public?


Discussion / hints:
I wanted the tests to run independent of buildkite, yes. My preference would be to leave them as GitHub Actions, but it's totally up to you :)
@dianaclarke Thank you for the quick response. I am leaving this as it is for now until we have more good reasons.
>  My preference would be to leave them as GitHub Actions,

I also quite like GHA for the integration with the PR workflow. A downside of BK is when external contributors chime in -- we've always had some workflow struggles around that (builds not triggering, and more).

The big advantage of BK is when CI is really resource intense -- with BK, one can fully take control of the underlying infrastructure. To me it seems like the current conbench CI is lightweight enough and kind of perfectly suited for running on GHA.
I am in favor of removing one of both rather sooner than later. So that we learn what we're potentially missing out on. I'd say in the current state of the system it really makes sense to remove the BK CI from PRs.
Just for completeness sake/another vote for GHA:

> The big advantage of BK is when CI is really resource intense -- with BK, one can fully take control of the underlying infrastructure. 

GHA has two different solutions for this now: self-hosted runners and [pay-as-you-go larger, github-hosted runners](https://docs.github.com/en/actions/using-github-hosted-runners/using-larger-runners)
+1 on removing BK. Now that we have uncoupled the `conbench-test` BK pipeline from the `conbench-deploy` one, it seems there's not as much a reason to keep the BK one around.
I'm also a +1 on simplifying | unifying on GHA. One additional thing that's nice about GHA (I'm sure we could make buildkite do this, somehow, but AFAIK it doesn't out of the box): we get actions on forks for external contributors
I am good if we disable BK test pipelines as long as:
1. GHA tests migrations the same way BK does.
2. We auto-deploy only after tests pass although since we no longer auto-deploy into Production, it might be ok. 
3. We runs tests agains main branch before deploying anything in Prod.

Agreed; great considerations. I'll add that for (3), the BK deploy pipelines could simply hit `https://api.github.com/repos/conbench/conbench/commits/<COMMIT_SHA>/check-runs?check_name=build` in order to see if that commit has passed GHA tests.
@ElenaHenderson let me try to walk through your three points to see if we got them covered.

> GHA tests migrations the same way BK does.

https://github.com/conbench/conbench/blob/ddf496c77a7624fe3087c82fe53e7ec4e78b134c/.buildkite/conbench-test/pipeline.yml#L13

BK:
```
  - label: "Test Migrations"
    key: "test-migrations"
    depends_on: "test"
    command:
      - docker-compose down
      - docker-compose run migration
```
GHA:
```
      - name: Migrations
        run: docker-compose run -e CREATE_ALL_TABLES app alembic upgrade head
        env:
          CREATE_ALL_TABLES: false
```
(https://github.com/conbench/conbench/blob/main/.github/workflows/actions.yml#L37)

defined here:
https://github.com/conbench/conbench/blob/ddf496c77a7624fe3087c82fe53e7ec4e78b134c/docker-compose.yml#L58

I think these are equivalent, but for clarity I think we should look into either using `docker-compose run migration` again (the abstraction) or remove that thing from the docker-compose file (as it's otherwise untested).

> We auto-deploy only after tests pass although since we no longer auto-deploy into Production, it might be ok.

Auto-deploy to staging environment currently happens after merge to main, which _should_ imply that tests passed _on the PR_.

I think you mean that before that auto-deploy to happen you'd love to see another round of tests pass on the `main` branch. 

That is indeed currently not the case. I think that is OK for now, but if we want to change this in the future: 

The first step in `conbench-staging-deploy/pipeline.yml` should then try to read the GitHub Actions CI check status of HEAD of `main`, and fail fast if that check is red, wait until it's green if its in progress, and proceed if it's green. That sounds like complicated to build, but I suppose that's what needs to be done when trying to combine two separate build systems.

> We runs tests agains main branch before deploying anything in Prod.

I think that's similar to point (2), and I suppose the serious serious prod deployment is anyway triggered manually.
Thanks for all the feedback everyone. I have renamed the issue to reflect the action item we agreed on. Working on this now.
I wanted the tests to run independent of buildkite, yes. My preference would be to leave them as GitHub Actions, but it's totally up to you :)
@dianaclarke Thank you for the quick response. I am leaving this as it is for now until we have more good reasons.
>  My preference would be to leave them as GitHub Actions,

I also quite like GHA for the integration with the PR workflow. A downside of BK is when external contributors chime in -- we've always had some workflow struggles around that (builds not triggering, and more).

The big advantage of BK is when CI is really resource intense -- with BK, one can fully take control of the underlying infrastructure. To me it seems like the current conbench CI is lightweight enough and kind of perfectly suited for running on GHA.
I am in favor of removing one of both rather sooner than later. So that we learn what we're potentially missing out on. I'd say in the current state of the system it really makes sense to remove the BK CI from PRs.
Just for completeness sake/another vote for GHA:

> The big advantage of BK is when CI is really resource intense -- with BK, one can fully take control of the underlying infrastructure. 

GHA has two different solutions for this now: self-hosted runners and [pay-as-you-go larger, github-hosted runners](https://docs.github.com/en/actions/using-github-hosted-runners/using-larger-runners)
+1 on removing BK. Now that we have uncoupled the `conbench-test` BK pipeline from the `conbench-deploy` one, it seems there's not as much a reason to keep the BK one around.
I'm also a +1 on simplifying | unifying on GHA. One additional thing that's nice about GHA (I'm sure we could make buildkite do this, somehow, but AFAIK it doesn't out of the box): we get actions on forks for external contributors
I am good if we disable BK test pipelines as long as:
1. GHA tests migrations the same way BK does.
2. We auto-deploy only after tests pass although since we no longer auto-deploy into Production, it might be ok. 
3. We runs tests agains main branch before deploying anything in Prod.

Agreed; great considerations. I'll add that for (3), the BK deploy pipelines could simply hit `https://api.github.com/repos/conbench/conbench/commits/<COMMIT_SHA>/check-runs?check_name=build` in order to see if that commit has passed GHA tests.
@ElenaHenderson let me try to walk through your three points to see if we got them covered.

> GHA tests migrations the same way BK does.

https://github.com/conbench/conbench/blob/ddf496c77a7624fe3087c82fe53e7ec4e78b134c/.buildkite/conbench-test/pipeline.yml#L13

BK:
```
  - label: "Test Migrations"
    key: "test-migrations"
    depends_on: "test"
    command:
      - docker-compose down
      - docker-compose run migration
```
GHA:
```
      - name: Migrations
        run: docker-compose run -e CREATE_ALL_TABLES app alembic upgrade head
        env:
          CREATE_ALL_TABLES: false
```
(https://github.com/conbench/conbench/blob/main/.github/workflows/actions.yml#L37)

defined here:
https://github.com/conbench/conbench/blob/ddf496c77a7624fe3087c82fe53e7ec4e78b134c/docker-compose.yml#L58

I think these are equivalent, but for clarity I think we should look into either using `docker-compose run migration` again (the abstraction) or remove that thing from the docker-compose file (as it's otherwise untested).

> We auto-deploy only after tests pass although since we no longer auto-deploy into Production, it might be ok.

Auto-deploy to staging environment currently happens after merge to main, which _should_ imply that tests passed _on the PR_.

I think you mean that before that auto-deploy to happen you'd love to see another round of tests pass on the `main` branch. 

That is indeed currently not the case. I think that is OK for now, but if we want to change this in the future: 

The first step in `conbench-staging-deploy/pipeline.yml` should then try to read the GitHub Actions CI check status of HEAD of `main`, and fail fast if that check is red, wait until it's green if its in progress, and proceed if it's green. That sounds like complicated to build, but I suppose that's what needs to be done when trying to combine two separate build systems.

> We runs tests agains main branch before deploying anything in Prod.

I think that's similar to point (2), and I suppose the serious serious prod deployment is anyway triggered manually.
Thanks for all the feedback everyone. I have renamed the issue to reflect the action item we agreed on. Working on this now.

Fix patch:
diff --git a/.buildkite/conbench-staging-deploy/pipeline.yml b/.buildkite/conbench-staging-deploy/pipeline.yml
index fc4094719..2ad54c64b 100644
--- a/.buildkite/conbench-staging-deploy/pipeline.yml
+++ b/.buildkite/conbench-staging-deploy/pipeline.yml
@@ -1,4 +1,7 @@
 steps:
+  # TODO: use GitHub API to find if this commit had all CI checks passing.
+  # Abort deployment if that is not the case. Also see
+  # https://github.com/conbench/conbench/issues/319
   - label: "Build & Push"
     key: "build-and-push"
     concurrency: 1
diff --git a/.github/workflows/actions.yml b/.github/workflows/actions.yml
index cf37df3ee..d935a74d4 100755
--- a/.github/workflows/actions.yml
+++ b/.github/workflows/actions.yml
@@ -34,7 +34,7 @@ jobs:
         run: docker-compose run --detach db
       # Run `alembic upgrade head` via docker-compose so that it is within
       # the network that the DB is also in, reachable via DNS name `db`
-      - name: Migrations
+      - name: Test database migrations
         run: docker-compose run -e CREATE_ALL_TABLES app alembic upgrade head
         env:
           CREATE_ALL_TABLES: false
diff --git a/docker-compose.yml b/docker-compose.yml
index 734cbcee8..1d1fc254c 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -54,25 +54,3 @@ services:
       interval: 10s
       timeout: 5s
       retries: 5
-
-  migration:
-    build:
-      context: .
-      dockerfile: Dockerfile
-    command: ["alembic", "upgrade", "head"]
-    environment:
-      APPLICATION_NAME: "conbench-for-pytest"
-      CREATE_ALL_TABLES: "false"
-      DB_USERNAME: "postgres"
-      DB_HOST: "db"
-      DB_PASSWORD: "postgres"
-      DB_NAME: "postgres"
-      DB_PORT: "5432"
-      FLASK_APP: "conbench"
-      FLASK_ENV: "development"
-      REGISTRATION_KEY: "code"
-      SECRET_KEY: "Person, woman, man, camera, TV"
-      CONBENCH_INTENDED_BASE_URL: http://127.0.0.1:5000/
-    depends_on:
-      db:
-        condition: service_healthy


Test patch:
diff --git a/.buildkite/conbench-test/pipeline.yml b/.buildkite/conbench-test/pipeline.yml
deleted file mode 100644
index 9ae2c179b..000000000
--- a/.buildkite/conbench-test/pipeline.yml
+++ /dev/null
@@ -1,28 +0,0 @@
-steps:
-  - label: "Build"
-    key: "build"
-    command:
-      - docker-compose down
-      - docker-compose build
-
-  - label: "Test"
-    key: "test"
-    depends_on: "build"
-    command: docker-compose run -e BUILDKITE=true app pytest -vv -s conbench/tests/
-
-  - label: "Test Migrations"
-    key: "test-migrations"
-    depends_on: "test"
-    command:
-      - docker-compose down
-      - docker-compose run migration
-
-  # This makes sure that deploys are triggered in the same order as the
-  # test builds, no matter which test builds finish first.
-  - label: "Concurrency Gate"
-    key: "concurrency-gate"
-    depends_on: "test-migrations"
-    if: build.branch == 'main'
-    command: "exit 0"
-    concurrency: 1
-    concurrency_group: "conbench-deploy"