-
Notifications
You must be signed in to change notification settings - Fork 13
Complement: Add Synapse Workers build step #123
Conversation
a1a2056
to
2530a03
Compare
# Build the Synapse Workers docker image, which is located at: | ||
# https://github.com/matrix-org/synapse/blob/master/docker/Docker-workers | ||
# Note that this image is based on matrixdotorg/synapse:latest, which we're just | ||
# pulling from docker hub | ||
- wget https://github.com/matrix-org/synapse/archive/master.tar.gz | ||
- tar -xzf master.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need the whole of the synapse repo? If it's just the dockerfile you're after, just curl that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the dockerfile depends on additional files, named docker/conf/worker.yaml.j2 and docker/configure_workers_and_start.py.
It's probably easier to download the whole repo instead of the three files individually (which may become more files in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough.
Note that this incantation won't work until the dockerfile in question is present on synapse master
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On which note:
I have confirmed that this pipeline succeeds locally.
are you sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think abunch of
complement/pipeline.yml
Outdated
# Build the Synapse Workers docker image, which is located at: | ||
# https://github.com/matrix-org/synapse/blob/master/docker/Docker-workers | ||
# Note that this image is based on matrixdotorg/synapse:latest, which we're just | ||
# pulling from docker hub | ||
- wget https://github.com/matrix-org/synapse/archive/master.tar.gz | ||
- tar -xzf master.tar.gz | ||
- docker build -t matrixdotorg/synapse:workers -f synapse-master/docker/Dockerfile-workers synapse-master/ | ||
# Build the accompanying Synapse workers Complement docker container. This specifically | ||
# sets up a Synapse worker setup for Complement, and is located at: | ||
# https://github.com/matrix-org/complement/blob/master/dockerfiles/SynapseWorkers.Dockerfile. | ||
- docker build -t complement-synapse -f dockerfiles/SynapseWorkers.Dockerfile dockerfiles/ | ||
# Run the tests! | ||
- COMPLEMENT_BASE_IMAGE=complement-synapse go test -v -tags synapse_blacklist ./tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this script could go in the complement repo, though it's not necessarily long enough to be worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep it in here for now, so we have one place for the steps in the Complement pipeline and Synapse pipeline.
The wget
and tar
steps could probably be combined into one line as well. If it starts ballooning then moving it sounds more plausible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep it in here for now, so we have one place for the steps in the Complement pipeline and Synapse pipeline.
well, they're in separate pipeline files, so I'm not sure that's much of a reason. Still, it's fine for now.
complement/pipeline.yml
Outdated
@@ -62,7 +62,57 @@ steps: | |||
mount-buildkite-agent: false | |||
# Complement needs to know if it is running under CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate comment... I feel like I've said this before somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. I removed one instance, but there was another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think abunch of
what I meant to write here but apparently failed was that a bunch of the comments on #122 also apply here (also: vice versa, re COMPLEMENT_BASE_IMAGE
).
I'd like to see a decision on matrix-org/complement#65.
Yeah, I've been applying some fixes to both sides as a result of your comments. It took me a while to figure out YAML anchors, but now that I'm a bit more confident in using them I'm trying to apply them to both pipelines.
Noted, alright. |
What's the status of this PR @anoadragon453 ? |
@kegsay blocked on matrix-org/synapse#9162, which is hopefully very nearly there at this point. |
That has merged now, what's the status of this PR? |
Blocked on Synapse Worker image not being flaky on startup: matrix-org/synapse#10065 |
closing this, because complement has moved its CI to GHA. |
This PR extends the Complement pipeline to run tests on Synapse in worker mode. This PR is ready for review, but should not be merged until the relevant Synapse and Complement PRs are.
I have confirmed that this pipeline succeeds locally.