Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from Buildkite to GitHub Actions #1115

Closed
wants to merge 60 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
469707c
First attempt at a GHA pipeline
Aug 11, 2021
74bef07
Provide dummy "run"; parameterize homeserver impl
Aug 11, 2021
4c86c1a
Try to force GHA to trigger in my fork
Aug 11, 2021
f68bbe4
correct yaml nesting; fix image name
Aug 11, 2021
6f8197f
Flesh out test cases and setup env
Aug 11, 2021
c4afc4f
GHA contexts require single quotes around strings
Aug 11, 2021
40e9979
Names are redundant: don't use `yes`
Aug 11, 2021
ed0d9d7
Maybe setting working dir will help?
Aug 11, 2021
3142295
Mount . as /sytest
Aug 11, 2021
f93a663
`.` was too short. Workspace as full path?
Aug 11, 2021
78814e0
Guess to try and fix the logs path
Aug 11, 2021
47ae955
single quotes :(
Aug 11, 2021
f9ac100
bash -xe for debug info
Aug 11, 2021
d87a25e
more -xe
Aug 11, 2021
1e98e8d
More exploratory debugging
Aug 11, 2021
4a66f6d
Try an explicit cat?
Aug 11, 2021
d87f6a8
Comment out to make the pipeline simpler
Aug 11, 2021
7436223
Where are we doing the tarring in?
Aug 11, 2021
1e8f5ad
synapse_sytest: use SYNAPSE_SOURCE throughout
Aug 11, 2021
04329ae
Can we get sytest running?
Aug 11, 2021
06f684b
stupid debug
Aug 11, 2021
bc453ba
desperation printf debugging
Aug 11, 2021
0567858
echo with quotes!?!?!
Aug 11, 2021
1171129
Remove bootstrap debug for now; mount /src
Aug 11, 2021
b8a7175
I think it'll be easier to concentrate on synapse
Aug 11, 2021
8ff4b2d
Again
Aug 11, 2021
5ed4665
Revert a bunch of incorrect changes; mkdir -p
Aug 11, 2021
2302479
Tweak pipeline
Aug 11, 2021
5aab2fe
Tweak checkout
Aug 11, 2021
ec135d6
Explicit paths
Aug 11, 2021
2f0c6d2
Fix blacklist path
Aug 11, 2021
cfb8729
Try to get sytest logs uploaded as artefacts
Aug 11, 2021
b78fad2
update TODOs; try more synapse testing
Aug 11, 2021
0862c20
Try to checkout the corresponding synapse branch
Aug 12, 2021
45cbb1a
Use rest API to avoid checkouts action retrying
Aug 12, 2021
ccd4e89
Need API token to make an API request hurrrr
Aug 12, 2021
7eafc11
Log annotation already handled by tap_to_gha.pl
Aug 12, 2021
66f1ac5
ci -> .ci
Aug 12, 2021
d2e27df
Remove backticks
Aug 12, 2021
58aabb9
dendrite
Aug 12, 2021
c74afbd
Fix docker volume cfg to mount dendrite
Aug 12, 2021
13f9000
I got lost in the yaml
Aug 12, 2021
bc2b9ce
Try to reduce annotation spam
Aug 12, 2021
aec7fa6
Okay, that didn't work because no curl
Aug 12, 2021
1a6a46c
Missing quote
Aug 12, 2021
cfda89b
Fix branches and apply to dendrite
Aug 12, 2021
0dc6a18
Oh, workers was an additional blacklist, not a replacement
Aug 12, 2021
03a59bf
Don't special case the github-actions branch
Aug 12, 2021
fd51fc4
Fix "fetch dendrite branch" step name
DMRobertson Aug 13, 2021
2984f8f
Add explanatory comments
Aug 13, 2021
9e4a8f3
Try to get friendly job names
Aug 13, 2021
b5350ec
TEMP: hack in github-actions branch again
Aug 13, 2021
dd7b896
Fiddle with labels; include, `&& 1` for dendrite
Aug 13, 2021
06e00d5
Mark annotate.md as bk-specific
Aug 13, 2021
8589838
Shorten synapse labels
Aug 13, 2021
ea25d7a
More label faffing
Aug 13, 2021
7a9ca4b
Use BUILDKITE_LABEL for annotations again
Aug 13, 2021
0994d0c
Fix and improve handling of env vars
Aug 18, 2021
ce29fb2
More descriptive blacklist step name
Aug 18, 2021
5692410
We do actually want CI to pass a branch
Aug 19, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions .github/workflows/pipeline.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
name: Run sytest
on:
push:
branches: ["develop", "release-*", "github-actions"]
pull_request:

# Only run this action once per pull request/branch; restart if a new commit arrives.
# C.f. https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#concurrency
# and https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#github-context
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

jobs:
synapse:
name: "Synapse: ${{ matrix.label }}"
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
include:
- label: Py 3.6, SQLite, Monolith
sytest-tag: bionic

- label: Py 3.6, PG 10, Monolith
sytest-tag: bionic
postgres: postgres

- label: Py 3.6, PG 10, Workers
sytest-tag: bionic
postgres: postgres
workers: workers

- label: Py 3.9, PG 13, Monolith
sytest-tag: testing
postgres: postgres

- label: Py 3.9, PG 13, Workers
sytest-tag: testing
postgres: postgres
workers: workers

container:
image: matrixdotorg/sytest-synapse:${{ matrix.sytest-tag }}
volumes:
# bootstrap.sh expects the sytest source available at /sytest.
# TODO Buildkite mounted /sytest as readonly. Can we do this on GHA? Do we need it?
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
- ${{ github.workspace }}/sytest:/sytest
# synapse_sytest.sh expects a synapse checkout at /src
- ${{ github.workspace }}/synapse:/src
env:
POSTGRES: ${{ matrix.postgres && 1 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is && 1 a trick to say that this should be enabled by default?

Why don't we do the same for dendrite's runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. With that said, it looks to me that dendrite_sytest.sh only ever uses postgres. Maybe the BuildKite config I've been adapting was mistaken?

The script always sets up postgres. Some other bits of perl then determine if we're going to actually use postgres. Need to ensure $POSTGRES is non-empty iff we want to use SQLite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure if I understand. If POSTGRES is empty, then we set it to true/1 instead...?

Is the same required for BLACKLIST and API?

Copy link
Contributor Author

@DMRobertson DMRobertson Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. I think I wrote out a larger comment but that seems to have disappeared.

I think it might be helpful to work through the details from scratch.

What the vars actually do

POSTGRES

POSTGRES is handled differently for synapse and dendrite.

  • In synapse_sytest.sh we only read POSTGRES in tests [ -n "$POSTGRES" ]. We need POSTGRES to be empty or not set for SQLite, and nonempty to use postgres.
  • In Dendrite.pm we test if POSTGRES is not defined or equal to '0', and use SQLlite otherwise.

Taking both behaviours into account:

  • to use postgres, set POSTGRES to any non-empty string other than '0'
  • to use sqllite, don't define POSTGRES

WORKERS

Synapse only. Tested with [ -n "$WORKERS" ].

API

Only for dendrite. If $ENV{'API'} == '1' in Dendrite.pm, we'll pass -api to dendrite. Not sure how that will handle the var being undefined.

BLACKLIST

Only for synapse. If "$BLACKLIST" is empty (test -z) then we set BLACKLIST=sytest-blacklist. It ends up getting passed to run-tests.pl via -B.

GitHub Actions' expression language

The language is typed, but presumably ${{ expr }} converts expr to a string before substituting it into the yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the best way to experiment was to run a dummy action here: https://github.com/DMRobertson/gha-test/

As far as I can tell, a && b returns a if a is falsey, otherwise b. No type conversion done on the value that gets returned.

The two cases then:

  • when postgres isn't included in the strategy matrix

    • matrix.postgres is null
    • null && 1 evaluates to null.
    • ${{ null }} subsitutes the text null into the yaml file.
    • we end up with a yaml KV pair POSTGRES: null
    • GHA interprets this as "don't set POSTGRES"
  • when postgres: postgres is in the strategy matrix

    • matrix.postgres is 'postgres'
    • 'postgres && 1 is 1
    • ${{ 1 }} substitutes the text null into the yaml file
    • yaml KV POSTGRES: 1
    • GHA sets the POSTGRES env var to the string 1

I'm about to add a commit which confirms this by dumping the environment before we call the bootstrap script.

WORKERS: ${{ matrix.workers && 1 }}
BLACKLIST: ${{ (matrix.workers && 'synapse-blacklist-with-workers') || 'sytest-blacklist' }}

steps:
- name: Checkout sytest
uses: actions/checkout@v2
with:
path: sytest

- name: Fetch corresponding synapse branch
shell: bash
run: |
BRANCH=${GITHUB_REF#refs/heads/}
(wget -O - https://github.com/matrix-org/synapse/archive/$BRANCH.tar.gz || wget -O - https://github.com/matrix-org/synapse/archive/develop.tar.gz) \
| tar -xz --strip-components=1 -C /src/

- name: Prepare blacklist file for running with workers
if: ${{ matrix.workers }}
run: cat /src/sytest-blacklist /src/.ci/worker-blacklist > /src/synapse-blacklist-with-workers

- name: Run sytest
run: |
echo POSTGRES=${POSTGRES:-<NOT SET>}
echo WORKERS=${WORKERS:-<NOT SET>}
echo BLACKLIST=${BLACKLIST:-<NOT SET>}
bash -xe /bootstrap.sh synapse

- name: Summarise results.tap
# Use always() to run this step even if previous ones failed.
if: ${{ always() }}
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
run: /sytest/scripts/tap_to_gha.pl /logs/results.tap

- name: Upload SyTest logs
uses: actions/upload-artifact@v2
if: ${{ always() }}
with:
name: Sytest Logs - ${{ job.status }} - (Synapse, ${{ matrix.label }})
path: |
/logs/results.tap
/logs/**/*.log*

dendrite:
runs-on: ubuntu-latest
name: "Dendrite: ${{ matrix.label }}"
strategy:
fail-fast: false
matrix:
include:
- label: SQLite

- label: SQLite, full HTTP APIs
api: full-http

- label: Postgres
postgres: postgres

- label: Postgres, full HTTP APIs
postgres: postgres
api: full-http

container:
image: matrixdotorg/sytest-dendrite
volumes:
# bootstrap.sh expects the sytest source available at /sytest.
# TODO Buildkite mounted /sytest as readonly. Can we do this on GHA? Do we need it?
- ${{ github.workspace }}/sytest:/sytest
# synapse_sytest.sh expects a synapse checkout at /src
- ${{ github.workspace }}/dendrite:/src
env:
POSTGRES: ${{ matrix.postgres && 1 }}
API: ${{ matrix.api && 1 }}

steps:
- name: Checkout sytest
uses: actions/checkout@v2
with:
path: sytest

- name: Fetch corresponding dendrite branch
shell: bash
run: |
BRANCH=${GITHUB_REF#refs/heads/}
(wget -O - https://github.com/matrix-org/dendrite/archive/$BRANCH.tar.gz || wget -O - https://github.com/matrix-org/dendrite/archive/master.tar.gz) \
| tar -xz --strip-components=1 -C /src/

- name: Run sytest
run: |
echo POSTGRES=${POSTGRES:-<NOT SET>}
echo API=${API:-<NOT SET>}
bash -xe /bootstrap.sh dendrite

- name: Summarise results.tap
if: ${{ always() }}
run: /sytest/scripts/tap_to_gha.pl /logs/results.tap

- name: Upload SyTest logs
uses: actions/upload-artifact@v2
if: ${{ always() }}
with:
name: Sytest Logs - ${{ job.status }} - (Dendrite, ${{ join(matrix.*, ', ') }})
path: |
/logs/results.tap
/logs/**/*.log*
10 changes: 4 additions & 6 deletions docker/bootstrap.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,18 @@ shift

if [ -d "/sytest" ]; then
# If the user has mounted in a SyTest checkout, use that.
# This is the case for sytest's GitHub Actions.
echo "Using local sytests"
else
echo "--- Trying to get same-named sytest branch..."

# Check if we're running in buildkite, if so it can tell us what
# Check if we're running in CI. If so it can tell us what
# Synapse/Dendrite branch we're running
if [ -n "$BUILDKITE_BRANCH" ]; then
branch_name=$BUILDKITE_BRANCH
if [ -n "$SYTEST_BRANCH" ]; then
branch_name="$SYTEST_BRANCH"
else
# Otherwise, try and find the branch that the Synapse/Dendrite checkout
# is using. Fall back to develop if unknown.
branch_name="$(git --git-dir=/src/.git symbolic-ref HEAD 2>/dev/null)" || branch_name="develop"
fi

if [ "$SYTEST_TARGET" == "dendrite" ] && [ "$branch_name" == "master" ]; then
# Dendrite uses master as its main branch. If the branch is master, we probably want sytest develop
branch_name="develop"
Expand Down
3 changes: 2 additions & 1 deletion scripts/dendrite_sytest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ echo >&2 "--- Copying assets"
# Copy out the logs
rsync -r --ignore-missing-args --min-size=1B -av /work/server-0 /work/server-1 /logs --include "*/" --include="*.log.*" --include="*.log" --exclude="*"

if [ $TEST_STATUS -ne 0 ]; then
# Generate annotate.md This is Buildkite-specific.
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
if [ -n "$BUILDKITE_LABEL" ] && [ $TEST_STATUS -ne 0 ]; then
# Build the annotation
perl /sytest/scripts/format_tap.pl /logs/results.tap "$BUILDKITE_LABEL" >/logs/annotate.md
# If show-expected-fail-tests logged something, put it into the annotation
Expand Down
7 changes: 4 additions & 3 deletions scripts/synapse_sytest.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/bash
#!/bin/bash -xe
#
# This script is run by the bootstrap.sh script in the docker image.
#
Expand All @@ -13,7 +13,7 @@ set -e

cd "$(dirname $0)/.."

mkdir /work
mkdir -p /work

# start the redis server, if desired
if [ -n "$REDIS" ]; then
Expand Down Expand Up @@ -201,7 +201,8 @@ rsync --ignore-missing-args --min-size=1B -av /work/server-0 /work/server-1 /log
#export TOP=/src
#/venv/bin/coverage combine

if [ $TEST_STATUS -ne 0 ]; then
# Generate annotate.md This is Buildkite-specific.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Generate annotate.md This is Buildkite-specific.
# Generate annotate.md. This is Buildkite-specific.

if [ -n "$BUILDKITE_LABEL" ] && [ $TEST_STATUS -ne 0 ]; then
# Build the annotation
perl /sytest/scripts/format_tap.pl /logs/results.tap "$BUILDKITE_LABEL" >/logs/annotate.md
fi
Expand Down