Correctly fetch same-named homeserver branches #1129
Conversation
DMRobertson
changed the title
|
Slightly anxious that dendrite fails on GHA but doesn't on BK. Flakey test? |
| # 2. Attempt to use the base branch, e.g. when merging into release-vX.Y | ||
| # (GITHUB_BASE_REF for pull requests). | ||
| # 3. Use the default synapse branch ("develop"). | ||
| for BRANCH_NAME in "$GITHUB_HEAD_REF" "$GITHUB_BASE_REF" "${GITHUB_REF#refs/heads/}" "develop"; do |
There was a problem hiding this comment.
Since we had an issue with "${GITHUB_REF#refs/heads/}" due to PR numbers, is it even worth including it on this list?
There was a problem hiding this comment.
GITHUB_HEAD_REF and GITHUB_BASE_REF only exist when the action is triggered by a pull request. https://docs.github.com/en/actions/reference/environment-variables#default-environment-variables. This action runs on specific branches too, outside of the context of pull requests:
name: Run sytest
on:
push:
branches: ["develop", "release-*"]
pull_request
(but note that's only there because I copied it from synapse's CI file)
Assuming it's meaningful to run sytest on develop and release branches, I think we do still want to read GITHUB_REF.
There was a problem hiding this comment.
Aha, I see. So in the case of a pull request, it's unlikely that we'll end up using "${GITHUB_REF#refs/heads/}". But it's useful on develop / release-*.
Yes, I think it's worthwhile to run Sytest on those branches.
| # TODO the shell script below is nicked from complement. We use this pattern | ||
| # in a few places. Can we make this an Action so it's easier to reuse? |
Most recent run seems to have fallen over with workers. Maybe the blacklist is still wrong? |
Maybe this will fix the worker tests?
|
Those failures seem to be replicated on buildkite. Maybe I need to update the corresponding synapse branch to pull in a fix there? /shrug |
|
That looks happier.
There we were talking about a dendrite failure ( Later we had https://github.com/matrix-org/sytest/runs/3527019236 where all dendrite tests suceeded. The most recent run is https://github.com/matrix-org/sytest/runs/3527233610 where that dendrite failure reoccurred. In both failure cases it was Dendrite with Postgres but without Full HTTP APIs that saw this. Some kind of race? |
There was a problem hiding this comment.
In both failure cases it was Dendrite with Postgres but without Full HTTP APIs that saw this. Some kind of race?
Hmm, it does look that way, though I can't quickly tell you why that might be.
Perhaps worth raising on issue on the Dendrite repo. Otherwise I think this PR as it stands is good to go.
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.