Add plugin support. #941
Add plugin support. #941
Conversation
|
@valkum Were you hoping for feedback on this or does it need more work (noting that this is a draft PR)? |
The SyTest part is tested, but I could not test the bootstrap.sh part in a container yet because of failing builds for the base image. Mentioned that in the homerserver-dev matrix room. So this needs both: Work and Feedback :) |
valkum
force-pushed
the
feature-plugins
branch
3 times, most recently
from
7dc1617
to
e61a412
Compare
bootstrap.sh will now download plugins given in the env var PLUGIN The format is <repo>/<plugin> and only github is support for now. They get downloaded into /sytest/plugins by default. Multiple plugins can be given, devided by : (similar to PATH) SyTest will now look for plugins in /sytest/plugins (overridable by $SYTEST_PLUGINS) ServerImplementations and Output plugins are supported for now. The dir structure of plugins follows the structure of sytest itself. See http://github.com/valkum/sytest_conduit for an example plugin
valkum
force-pushed
the
feature-plugins
branch
from
e61a412
to
e962cd0
Compare
|
I finished the missing work and tested it locally. Ready for review |
|
Could you give us an understanding of what the usecase for plugins in Sytest would be? I took a look at https://github.com/valkum/sytest_conduit, and it seems add files to allow SyTest to spin up and run against a Conduit instance. Could you not instead simply PR those files to this repo to add support instead? |
|
Submitting a PR for Conduit would be another way. But depending on how many Homeserver implementations will get created over time I thought this would be a better way. If you are more interested in simply adding Conduit support to SyTest its fine. I would submit a PR with our Homeserver implementation then. |
There was a problem hiding this comment.
I think this is generally a nice approach. I've left a few comments on details of it.
Generally: as part of the work, please make sure the documentation is updated to match. In particular, I think the top-level README and https://github.com/matrix-org/sytest/blob/develop/docker/README.md will need an update to document the new features, and the expected layout of the plugins directory/archives.
docker/bootstrap.sh
Outdated
| if [ -n "$PLUGIN" ]; then | ||
| mkdir /sytest/plugins | ||
| echo "--- Downloading plugins for sytest" | ||
| IFS=':'; for plugin in $PLUGIN; do |
There was a problem hiding this comment.
this should probably be called PLUGINS, if it's going to support more than one plugin?
There was a problem hiding this comment.
Yeah. I thought of the PATH var while writing it not sure why that is not called PATHS, but PLUGINS makes more sense. Will change that.
docker/bootstrap.sh
Outdated
| echo "--- Downloading plugins for sytest" | ||
| IFS=':'; for plugin in $PLUGIN; do | ||
| wget -q https://github.com/$plugin/archive/master.tar.gz -O plugin.tar.gz || { | ||
| echo "Failed to download plugin: $plugin" |
There was a problem hiding this comment.
we write this (to stdout rather than stderr) and then carry on with mkdir and tar anyway? seems like we should exit instead? or just let set -e handle it?
There was a problem hiding this comment.
Thanks for finding this. Yeah we should exit there with sending the msg to stderr.
|
I answered some of your feedback. I will add documentation when we decided on the exact format of dirs and variables. |
|
@valkum are you still interested in working on this? |
richvdh
added
the
X-Awaiting-Changes
|
Yeah, wrote kegsay in the complement room that I am currently at crunchtime and can pick up work on this (and the one in complement) after the 26th. |
Remove github tie in. Plugins can be tar.gz files hosted anywhere now. Added README info.
|
right, one remaining thing to do: please could you read https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.md#sign-off and add a comment below with a "signed-off-by" line, to confirm you agree to the statement there? |
|
[CI failed due to matrix-org/synapse#8785, which is fixed in #984) |
|
signed-off-by: Rudi Floren rudi.floren@gmail.com |
This PR adds support for plugins in sytest. The bootstrap script will download given plugins and sytest will use them.
You can now pass the env var $PLUGIN with the format
PLUGINS="<owner>/<repo>[:<owner>/<repo>]"bootstrap.sh (when downloading sytest) will load
<owner>/<repo>from github to/sytest/plugins/<owner>/<repo>and will look if a matching runner script is found in one of the plugins.
Example:
Sytest will look for Output and ServerImplementation plugins in the plugin dir.
Plugin dir is
/sytest/pluginby default but can be overwritten by the env var$SYTEST_PLUGINS