Add plugin support. #941

valkum · 2020-08-26T18:30:22Z

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:

$ PLUGIN="valkum/conduit_sytest" bootstrap.sh "conduit"
$ # Downloads https://github.com/valkum/conduit_sytest to /sytest/plugins/valkum/sytest 
$ # and executes /sytest/plugins/valkum/sytest/scrypts/conduit_sytest.sh

Sytest will look for Output and ServerImplementation plugins in the plugin dir.
Plugin dir is /sytest/plugin by default but can be overwritten by the env var $SYTEST_PLUGINS

clokep · 2020-08-27T11:24:30Z

@valkum Were you hoping for feedback on this or does it need more work (noting that this is a draft PR)?

valkum · 2020-08-27T11:40:53Z

@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.
Wanted to get some feedback before I invest more time into testing.

So this needs both: Work and Feedback :)

run-tests.pl

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 · 2020-09-17T12:27:49Z

I finished the missing work and tested it locally. Ready for review

anoadragon453 · 2020-09-23T13:12:20Z

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?

valkum · 2020-09-23T14:22:09Z

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.
With plugin support different projects are able to add their own Homeserver implementations and Output formatters.

If you are more interested in simply adding Conduit support to SyTest its fine. I would submit a PR with our Homeserver implementation then.

richvdh

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.

run-tests.pl

docker/bootstrap.sh

richvdh · 2020-09-24T12:29:29Z

docker/bootstrap.sh

+    if [ -n "$PLUGIN" ]; then
+        mkdir /sytest/plugins
+        echo "--- Downloading plugins for sytest"
+        IFS=':'; for plugin in $PLUGIN; do


this should probably be called PLUGINS, if it's going to support more than one plugin?

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

richvdh · 2020-09-24T12:31:09Z

docker/bootstrap.sh

+        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"


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?

Thanks for finding this. Yeah we should exit there with sending the msg to stderr.

valkum · 2020-09-24T13:39:22Z

I answered some of your feedback. I will add documentation when we decided on the exact format of dirs and variables.

richvdh · 2020-10-12T15:07:07Z

@valkum are you still interested in working on this?

valkum · 2020-10-12T15:51:53Z

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.

richvdh

looks good, though the docs could do with a few tweaks.

thanks!

docker/bootstrap.sh

README.rst

richvdh

lgtm! thanks very much!

richvdh · 2020-11-27T12:51:23Z

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?

richvdh · 2020-11-27T13:23:38Z

[CI failed due to matrix-org/synapse#8785, which is fixed in #984)

valkum · 2020-11-27T16:07:32Z

signed-off-by: Rudi Floren rudi.floren@gmail.com

valkum commented Aug 27, 2020

View reviewed changes

run-tests.pl Outdated Show resolved Hide resolved

valkum force-pushed the feature-plugins branch 3 times, most recently from 7dc1617 to e61a412 Compare September 17, 2020 12:24

valkum force-pushed the feature-plugins branch from e61a412 to e962cd0 Compare September 17, 2020 12:25

Change upstream url and cleanup

0eac922

valkum marked this pull request as ready for review September 17, 2020 12:28

clokep requested a review from a team September 17, 2020 13:43

richvdh requested changes Sep 24, 2020

View reviewed changes

anoadragon453 requested a review from richvdh October 7, 2020 15:05

richvdh removed their request for review October 12, 2020 15:07

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Oct 12, 2020

Update according to PR feedback

1904c9a

Remove github tie in. Plugins can be tar.gz files hosted anywhere now. Added README info.

richvdh self-requested a review November 19, 2020 10:52

richvdh requested changes Nov 19, 2020

View reviewed changes

docker/bootstrap.sh Show resolved Hide resolved

README.rst Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

Add more verbose docs.

593c6dc

richvdh requested a review from a team November 22, 2020 16:29

richvdh reviewed Nov 27, 2020

View reviewed changes

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

49da38d

richvdh approved these changes Nov 27, 2020

View reviewed changes

add link to example

a945257

richvdh merged commit 86f3ed8 into matrix-org:develop Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin support. #941

Add plugin support. #941

valkum commented Aug 26, 2020

clokep commented Aug 27, 2020

valkum commented Aug 27, 2020 •

edited

Loading

valkum commented Sep 17, 2020

anoadragon453 commented Sep 23, 2020

valkum commented Sep 23, 2020

richvdh left a comment

richvdh Sep 24, 2020

valkum Sep 24, 2020

richvdh Sep 24, 2020

valkum Sep 24, 2020

valkum commented Sep 24, 2020

richvdh commented Oct 12, 2020

valkum commented Oct 12, 2020

richvdh left a comment

richvdh left a comment

richvdh commented Nov 27, 2020 •

edited

Loading

richvdh commented Nov 27, 2020

valkum commented Nov 27, 2020

Add plugin support. #941

Add plugin support. #941

Conversation

valkum commented Aug 26, 2020

clokep commented Aug 27, 2020

valkum commented Aug 27, 2020 • edited Loading

valkum commented Sep 17, 2020

anoadragon453 commented Sep 23, 2020

valkum commented Sep 23, 2020

richvdh left a comment

Choose a reason for hiding this comment

richvdh Sep 24, 2020

Choose a reason for hiding this comment

valkum Sep 24, 2020

Choose a reason for hiding this comment

richvdh Sep 24, 2020

Choose a reason for hiding this comment

valkum Sep 24, 2020

Choose a reason for hiding this comment

valkum commented Sep 24, 2020

richvdh commented Oct 12, 2020

valkum commented Oct 12, 2020

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Nov 27, 2020 • edited Loading

richvdh commented Nov 27, 2020

valkum commented Nov 27, 2020

valkum commented Aug 27, 2020 •

edited

Loading

richvdh commented Nov 27, 2020 •

edited

Loading