Move from Buildkite to GitHub Actions #1115

DMRobertson · 2021-08-11T14:01:14Z

Ended up with a fairly direct translation from Buildkite to GHA. I tried to make more use of actions and so on, but generally found this to be more trouble that it's worth.

https://github.com/DMRobertson/sytest/actions/runs/1124068606 is an example where I'd screwed up the blacklist for synapse with workers. But that shows how test failures will appear (annotations, with a link through to logs)
https://github.com/DMRobertson/sytest/actions/workflows/pipeline.yml has the complete history
https://github.com/DMRobertson/sytest/actions/runs/1124143216 is an example of a happy run---except I don't understand why the failing dendrite workflow was unhappy.

bootstrap script is in the sytest-* containers, and I was running their version instead of the version in the checkout. Confusing!!

Getting somewhere

was just to force actions to trigger in my fork.

DMRobertson · 2021-08-12T14:40:56Z

Suggest we keep using BuildKite for a bit and see how this fares---then can turn off the webhook and remove the config from matrix-org/pipelines

anoadragon453

I can't say I know why the Dendrite run is appearing to fail. I don't see where the tap formatting script would return something other than 0 - is that failure flaky or deterministic?

Overall this is looking great. The linked output is very clean, and the resulting pipeline file doubly so!

Most of my comments are just clarifications.

.github/workflows/pipeline.yml

anoadragon453 · 2021-08-13T10:18:59Z

.github/workflows/pipeline.yml

+        # synapse_sytest.sh expects a synapse checkout at /src
+        - ${{ github.workspace }}/synapse:/src
+      env:
+        POSTGRES: ${{ matrix.postgres && 1 }}


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

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

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.

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?

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.

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.

.github/workflows/pipeline.yml

scripts/dendrite_sytest.sh

.github/workflows/pipeline.yml

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

scripts/synapse_sytest.sh

Need to ensure that API and POSTGRES are the empty strings for dendrite when they're meant to be off. But ${{ matrix.api == 'full-http' && foo }} seems to yield either foo or `false`. Stick with what we know works.

but only on BUILDKITE. Keep this around until we're confident with the GHA workflow.

DMRobertson · 2021-08-13T14:56:21Z

@anoadragon453 Updated as per your comments, (many thanks).

These runs might be of interest:

https://github.com/DMRobertson/sytest/actions/runs/1127951256 - including a flakey failure?
https://github.com/DMRobertson/sytest/actions/runs/1128005188

- Don't use API for synapse; use right matrix key for WORKERS - echo env vars before we invoke bootstrap script

DMRobertson · 2021-08-18T12:22:54Z

Most recent run: https://github.com/DMRobertson/sytest/runs/3360789317?check_suite_focus=true

anoadragon453

Thanks for answering my query in so much detail! I'm afraid at the moment that your links seem to lead to a private repository that I can't access. I'd like to take a look at the CI runs before approving.

Thanks for tackling the other items as well. Very small nits below in the meantime.

scripts/dendrite_sytest.sh

anoadragon453 · 2021-08-25T13:45:49Z

scripts/synapse_sytest.sh

@@ -201,6 +201,7 @@ rsync --ignore-missing-args --min-size=1B -av /work/server-0 /work/server-1 /log
 #export TOP=/src
 #/venv/bin/coverage combine

+# Generate annotate.md This is Buildkite-specific.


Suggested change

# Generate annotate.md This is Buildkite-specific.

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

anoadragon453 · 2021-08-25T13:47:12Z

.github/workflows/pipeline.yml

@@ -20,23 +20,23 @@ jobs:
      fail-fast: false
      matrix:
        include:
-          - label: Python 3.6 / SQLite / Monolith
+          - label: Py3.6 / SQLite / Monolith


bionic has SQLite 3.22.0 - it can be useful to list it, we've had issues with older SQLite's in the past.

DMRobertson · 2021-08-25T14:53:03Z

I'm afraid at the moment that your links seem to lead to a private repository that I can't access. I'd like to take a look at the CI runs before approving.

Oh bugger. I initially had a public fork where I was testing this stuff out. It looks like I've gone on to delete that fork, presumably when I realised the done thing was to make a branch within the main matrix-org repo. That is unfortunate and presumably means that all the actions runs are gone.

I think it might be best to push the branch I was working on directly to the main sytest repo and open a new pull request.

anoadragon453 · 2021-08-25T15:28:13Z

Oh, I've just realised I reviewed the other PR while it was marked as a draft. I'm not sure if that PR was intended just to show off the CI or whether we were moving discussion there.

Either way, ✔️ on the other PR.

DMRobertson · 2021-08-31T09:09:54Z

See the other PR, #1127

Suggest we leave Buildkite running in parallel for the time being. In particular we want to ensure that the cross-branch synapse-sytest and dendrite-sytest stuff works. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

In #1115 I didn't correctly handle the situation where there's a corresponding synapse/dendrite branch to fetch. #1128 fell victim to this. Fix: reuse the shell script from matrix-org/synapse#10160, i.e. consult a wider range of GHA's environment variables. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

David Robertson added 30 commits August 11, 2021 14:56

First attempt at a GHA pipeline

469707c

Provide dummy "run"; parameterize homeserver impl

74bef07

Try to force GHA to trigger in my fork

4c86c1a

correct yaml nesting; fix image name

f68bbe4

Flesh out test cases and setup env

6f8197f

GHA contexts require single quotes around strings

c4afc4f

Names are redundant: don't use yes

40e9979

Maybe setting working dir will help?

ed0d9d7

Mount . as /sytest

3142295

. was too short. Workspace as full path?

f93a663

Guess to try and fix the logs path

78814e0

single quotes :(

47ae955

bash -xe for debug info

f9ac100

more -xe

d87a25e

More exploratory debugging

1e98e8d

Try an explicit cat?

4a66f6d

Comment out to make the pipeline simpler

d87f6a8

Where are we doing the tarring in?

7436223

synapse_sytest: use SYNAPSE_SOURCE throughout

1e8f5ad

Can we get sytest running?

04329ae

stupid debug

06f684b

desperation printf debugging

bc453ba

echo with quotes!?!?!

0567858

Remove bootstrap debug for now; mount /src

1171129

bootstrap script is in the sytest-* containers, and I was running their version instead of the version in the checkout. Confusing!!

I think it'll be easier to concentrate on synapse

b8a7175

Again

8ff4b2d

Revert a bunch of incorrect changes; mkdir -p

5ed4665

Getting somewhere

Tweak pipeline

2302479

Tweak checkout

5aab2fe

Explicit paths

ec135d6

DMRobertson requested a review from a team August 12, 2021 14:13

Don't special case the github-actions branch

03a59bf

was just to force actions to trigger in my fork.

DMRobertson marked this pull request as ready for review August 13, 2021 09:43

anoadragon453 requested changes Aug 13, 2021

View reviewed changes

Fix "fetch dendrite branch" step name

fd51fc4

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

DMRobertson commented Aug 13, 2021

View reviewed changes

scripts/synapse_sytest.sh Outdated Show resolved Hide resolved

David Robertson added 8 commits August 13, 2021 14:51

Add explanatory comments

2984f8f

Try to get friendly job names

9e4a8f3

TEMP: hack in github-actions branch again

b5350ec

Fiddle with labels; include, && 1 for dendrite

dd7b896

Need to ensure that API and POSTGRES are the empty strings for dendrite when they're meant to be off. But ${{ matrix.api == 'full-http' && foo }} seems to yield either foo or `false`. Stick with what we know works.

Mark annotate.md as bk-specific

06e00d5

Shorten synapse labels

8589838

More label faffing

ea25d7a

Use BUILDKITE_LABEL for annotations again

7a9ca4b

but only on BUILDKITE. Keep this around until we're confident with the GHA workflow.

DMRobertson requested a review from anoadragon453 August 13, 2021 14:54

Fix and improve handling of env vars

0994d0c

- Don't use API for synapse; use right matrix key for WORKERS - echo env vars before we invoke bootstrap script

David Robertson added 2 commits August 18, 2021 13:24

More descriptive blacklist step name

ce29fb2

We do actually want CI to pass a branch

5692410

anoadragon453 reviewed Aug 25, 2021

View reviewed changes

DMRobertson mentioned this pull request Aug 25, 2021

Move from Buildkite to GitHub Actions, attempt 2 #1127

Merged

DMRobertson closed this Aug 31, 2021

DMRobertson mentioned this pull request Sep 6, 2021

Correctly fetch same-named homeserver branches #1129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from Buildkite to GitHub Actions #1115

Move from Buildkite to GitHub Actions #1115

DMRobertson commented Aug 11, 2021 •

edited

Loading

DMRobertson commented Aug 12, 2021

anoadragon453 left a comment

anoadragon453 Aug 13, 2021

DMRobertson Aug 13, 2021

anoadragon453 Aug 16, 2021

DMRobertson Aug 18, 2021 •

edited

Loading

DMRobertson Aug 18, 2021

DMRobertson commented Aug 13, 2021 •

edited

Loading

DMRobertson commented Aug 18, 2021

anoadragon453 left a comment

anoadragon453 Aug 25, 2021

anoadragon453 Aug 25, 2021

DMRobertson commented Aug 25, 2021

anoadragon453 commented Aug 25, 2021 •

edited

Loading

DMRobertson commented Aug 31, 2021

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

Move from Buildkite to GitHub Actions #1115

Move from Buildkite to GitHub Actions #1115

Conversation

DMRobertson commented Aug 11, 2021 • edited Loading

DMRobertson commented Aug 12, 2021

anoadragon453 left a comment

Choose a reason for hiding this comment

anoadragon453 Aug 13, 2021

Choose a reason for hiding this comment

DMRobertson Aug 13, 2021

Choose a reason for hiding this comment

anoadragon453 Aug 16, 2021

Choose a reason for hiding this comment

DMRobertson Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

What the vars actually do

POSTGRES

WORKERS

API

BLACKLIST

GitHub Actions' expression language

DMRobertson Aug 18, 2021

Choose a reason for hiding this comment

DMRobertson commented Aug 13, 2021 • edited Loading

DMRobertson commented Aug 18, 2021

anoadragon453 left a comment

Choose a reason for hiding this comment

anoadragon453 Aug 25, 2021

Choose a reason for hiding this comment

anoadragon453 Aug 25, 2021

Choose a reason for hiding this comment

DMRobertson commented Aug 25, 2021

anoadragon453 commented Aug 25, 2021 • edited Loading

DMRobertson commented Aug 31, 2021

DMRobertson commented Aug 11, 2021 •

edited

Loading

DMRobertson Aug 18, 2021 •

edited

Loading

`POSTGRES`

`WORKERS`

`API`

`BLACKLIST`

DMRobertson commented Aug 13, 2021 •

edited

Loading

anoadragon453 commented Aug 25, 2021 •

edited

Loading