Skip to content
This repository has been archived by the owner on Nov 25, 2022. It is now read-only.

Complement: Add Synapse Workers build step #123

Closed

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jan 21, 2021

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.

@anoadragon453 anoadragon453 requested review from kegsay and a team January 21, 2021 16:31
@anoadragon453 anoadragon453 force-pushed the anoa/complement_pipelines_synapse_workers branch from a1a2056 to 2530a03 Compare January 21, 2021 18:20
kegsay
kegsay previously approved these changes Jan 22, 2021
Comment on lines +77 to +82
# 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
Copy link
Member

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?

Copy link
Member Author

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).

Copy link
Member

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.

Copy link
Member

@richvdh richvdh Jan 25, 2021

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?

Copy link
Member

@richvdh richvdh left a 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

Comment on lines 77 to 89
# 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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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 Show resolved Hide resolved
complement/pipeline.yml Outdated Show resolved Hide resolved
@@ -62,7 +62,57 @@ steps:
mount-buildkite-agent: false
# Complement needs to know if it is running under CI
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@richvdh richvdh left a 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.

@anoadragon453
Copy link
Member Author

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).

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.

I'd like to see a decision on matrix-org/complement#65.

Noted, alright.

@kegsay
Copy link
Member

kegsay commented Apr 9, 2021

What's the status of this PR @anoadragon453 ?

@anoadragon453
Copy link
Member Author

@kegsay blocked on matrix-org/synapse#9162, which is hopefully very nearly there at this point.

@kegsay
Copy link
Member

kegsay commented May 14, 2021

That has merged now, what's the status of this PR?

@anoadragon453
Copy link
Member Author

Blocked on Synapse Worker image not being flaky on startup: matrix-org/synapse#10065

@richvdh
Copy link
Member

richvdh commented Jan 17, 2022

closing this, because complement has moved its CI to GHA.

@richvdh richvdh closed this Jan 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants