openwallet-foundation / acapy

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
409 stars 512 forks source link

Plugins Standards and Repository #2277

Closed usingtechnology closed 1 year ago

usingtechnology commented 1 year ago

Following @ianco work on plugins...

We've encountered issues with teams (Ontario) having no place to publicly share their plugins, we've encountered teams that have had loading issues with their plugins with new releases of ACA-Py, we've seen multiple different ways teams build their images with their plugins... The time has come to bring some kind of standards to developing and deploying/publishing plugins; promoting them, and making them easily accessible for other teams.

This is a parent / epic issue where we can track comments and add sub-tasks for related work.

The current thinking is there will be a repository separate from aries-cloudagent-python that will be a collection of plugins. They should be proven to work and follow a known way to be installed and configured.

usingtechnology commented 1 year ago

I have an outline repository that we can use for the discussion. I included the basicmessage_storage plugin from traction.

I had to add some integration tests to test out the various things devs may need to do and ensure they work inside devcontainers.

Things for plugin devs to do (for discussion):

  1. provide documentation/blurb at the top-level readme (basically an advertisement for their plugin)
  2. provide detailed documentation within their plugin sub-folder, detail configuration, use cases, if plugin has restrictions/dependencies etc.
  3. provide integration tests
  4. a devcontainer that allows running the integration tests, debugging/running their plugin

We can provide a template or a guide with the standards, so others know what is expected on a PR to include their plugin.

At the top-level, I would like to see some integration tests illustrating the plugins used in a production like image (pulled in to an ACA-Py image, configured and tested). Not sure how best to do that, but it is important to show (and prove) that if the plugin is loaded, configured and used in the "approved" way it "just works".

Asking for feedback and discussion from: @swcurran @esune @loneil @WadeBarnes @ianco @dbluhm @shaangill025. Please encourage any other plugin developers you can think of to join this discussion.

For me, a little elephant in the room is do we take on @ianco recommendations on how to configure/load plugins before doing this work? or after adding creating the repo? I am thinking more around the definition file and removing the setup function (which is how traction does all their plugins).

ianco commented 1 year ago

For me @usingtechnology 's repo looks good.

a little elephant in the room is do we take on @ianco recommendations on how to configure/load plugins before doing this work? or after adding creating the repo? I am thinking more around the definition file and removing the setup function (which is how traction does all their plugins).

I think if the goal is to get some standardization on existing plugins it makes sense to introduce as few changes as possible. Maybe some cleanup around the definition file and how plugins are loaded, if it's not going to make it too much trouble for people to get their plugins into a standardized format.

dbluhm commented 1 year ago

I like the idea of being able to run automated tests on lots of plugins for validating ACA-Py releases. I dislike the idea of PRs for plugin maintenance and features all going to the same repo. To be perfectly frank, slow turn around time on PRs and releases in HL repos is at least part of the reason why I often turn to writing features as plugins first.

I have a strong aversion to git submodules but maybe something using them would enable us to have a slightly more decentralized plugin ecosystem while also being able to test things. Or perhaps we could just have an integration test runner on an acapy plugin repo that dynamically pulled plugins and their integration tests and could run them all in the same place.

swcurran commented 1 year ago

I like the idea of being able to run automated tests on lots of plugins for validating ACA-Py releases. I dislike the idea of PRs for plugin maintenance and features all going to the same repo. To be perfectly frank, slow turn around time on PRs and releases in HL repos is at least part of the reason why I often turn to writing features as plugins first.

We need to fix that immediately!!! I think we are too quiet about waiting for “someone else” to review and merge PRs in ACA-Py. Let's be more aggressive about pinging maintainers to review PRs, and review with summarized comments (including — “Ready!” and “Hey XXXX, can you take a look as well?”), getting them merged. For me, I just don’t have the expertise to know when enough is enough on the review, so I (quietly…that’s wrong) leave space for others. Also make use of Draft Status to communicate where you are in the process, with explicit notes if you want reviews. In other words — lets be more deliberate about communicatiing.

Example — today’s PR on Public DIDs. @dbluhm , you approved it. I suggest either you immediately merge it, or you explicitly ask someone (e.g. @usingtechnology) to also review with any comments/context. As needed, discuss, and get merged.

Thoughts on the structure of the plugins — these could be dumb ideas:

Let’s get both of these topics — and a roadmap of immediate needs — on the agenda for ACA-Pug. Perhaps we also make a second, 1/2 hour devs/maintainers meeting to go over PRs and the roadmap on alternate weeks.

dbluhm commented 1 year ago

Let’s get both of these topics — and a roadmap of immediate needs — on the agenda for ACA-Pug. Perhaps we also make a second, 1/2 hour devs/maintainers meeting to go over PRs and the roadmap on alternate weeks.

I like this idea!

I think we are too quiet about waiting for “someone else” to review and merge PRs in ACA-Py. Let's be more aggressive about pinging maintainers to review PRs, and review with summarized comments (including — “Ready!” and “Hey XXXX, can you take a look as well?”), getting them merged.

Okay, point taken lol :slightly_smiling_face: I feel a reluctance to "step on toes" but I think explicitly making requests to others to review will help. (For the record, I was planning to tag others on that PR before you mentioned it lol. Should have been more prompt about it :smile: )

Have a “test deployments” repo, that has samples of deployments of combination of plugins people want to make, with some tests. Plugins can be in different places.

I think this is a good idea; though, an aries-cloudagent-python-plugins repo seems like the right place for this to live rather than a separate repo, in my opinion.

loneil commented 1 year ago

Probably not too much to add from me, the repo structure looks good and having a common place for them sounds like a good idea.

The concerns about possibly waiting for a change to a common, gated repository to get one of our plugins released is a good one. If I have a Traction instance and need to quickly change something in one of the plugins that make up Traction to get out quickly, or get feedback on in an environment would that still work? Or would changes have to be accepted and merged in to this new repo before being able to have them deployed as part of Traction....?

Specifically for Traction, PR based CICD is very useful where a change can be made and tested out in an OpenShift (specific to Traction again) deployment for stakeholder/team feedback and such. Maybe would have to work something else out for Traction PRs to have them point at a branch of the plugin repo?

esune commented 1 year ago

Regarding a single repo for plugins: as @swcurran noted, it will not be mandatory, but a decision made by the creator/maintainer of the plugin and the community.

The way I see it, the aries-cloudagent-python-plugins repo will/should host "official" plugins that build on top of ACA-Py and extend functionality that is not use-case specific: multi-tenancy and redis queue handling are two good examples, in the future it could be the support for different ledger types, the endorser functionality, etc.. Plugins that cater to a different audience (such as the part of traction which is specific to that use-case, there are several plugins in the repo right now) actually should be in a separate repo as they are part of a product. However, having this repo with examples and consolidated patterns to follow when implementing new functionality should reduce the occurrence of "interesting surprises" when running the code (if they are being followed, of course).

Last thought (I am unsure on whether this is even possible): having official plugins in a repo we could look into ways to publish them so that they can be installed using a package manager rather than copying code over (probably a longer term investigation/conversation).

usingtechnology commented 1 year ago

assuming that a big part of the plugins repo would be testing, can we test "external" plugins so they are "verified" too? so this repo would not only be a set of approved and well documented plugins but also a directory of other known-good plugins (hopefully well documented, but at least conforming to whatever pattern is required to be tested).

swcurran commented 1 year ago

I think there are two parts of testing.

First, there should be a way to for individual plugins to be tested. That’s up to the creator of the plugin, and should follow what ever techniques are possible/best practices. I don’t know how consistent that can be across plugins — e.g. is it a commonly named script/docker-compose setup in the plugin folder that can be invoked for testing?

Second, “we” (e.g., anyone that wants to put in the effort) should be able to construct a test that pulls in several plugins and executes some integration tests. @Jsyro used Aries Agent Test Harness to do that beautifully with the Redis plugin — spending a bit of time constructing the image and then just using the AATH to show that the tests run the same with/without the plugin. In these, we assume the plugins work, and we’re just demonstrating a specific set can be used together.

Does that make sense as a goal?

WadeBarnes commented 1 year ago

assuming that a big part of the plugins repo would be testing, can we test "external" plugins so they are "verified" too? so this repo would not only be a set of approved and well documented plugins but also a directory of other known-good plugins (hopefully well documented, but at least conforming to whatever pattern is required to be tested).

The official plugin repo could contain the reusable GitHub actions needed for generic plugin tests. External plugin repos could then integrate the actions into their CICD process and setup a nightly verification test run. This way the official set of steps need to verify a plugin are managed in the official plugin repo and can change over time as the requirements change. We made heavy use of reusable actions and workflows in the indy-plenum and indy-node workflows to perform all of the common steps. We were also able to reuse many of the actions and workflows on external implementations such as the Sovrin implementation. We extracted all of the common actions and workflows to the indy-shared-gha repo. But in this case it would make sense to maintain the reusable actions and workflows in the official plug-in repo.

External plugin developers could register (via PR) to have their plug-in included in a list of external plugins. I see this list being a table listing the name of the plugin, a link to the repo, and a badge indicating the status of it's nightly verification test run.

esune commented 1 year ago

The official plugin repo could contain the reusable GitHub actions needed for generic plugin tests. External plugin repos could then integrate the actions into their CICD process and setup a nightly verification test run. This way the official set of steps need to verify a plugin are managed in the official plugin repo and can change over time as the requirements change.

Thanks @WadeBarnes, this describes very well what I was thinking too 🙂