Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move plugins to use plugin repo #980

Merged
merged 8 commits into from
Jan 24, 2024
Merged

Move plugins to use plugin repo #980

merged 8 commits into from
Jan 24, 2024

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Jan 11, 2024

Going to go over this a couple more times but opening for visibility. Pretty sure everything is working well.

  • Downloads and installs the plugins from the remote repo.
  • Update all references.
  • Deletes the plugin source code
  • Leaves traction inkeeper and existing structure the same.

@jamshale jamshale marked this pull request as ready for review January 11, 2024 19:51
@jamshale jamshale requested review from loneil and esune January 11, 2024 23:05
@loneil
Copy link
Contributor

loneil commented Jan 12, 2024

Tried docker composing things up locally and it seems to be working there.
On the PR deployed from this the plugins might not be loading properly?
When trying tenant operations (logging in with innkeeper, creating a tenant from a reservation etc) I get errors like this on the OCP pod

aries_cloudagent.config.base.InjectionError: Error loading multitenant manager, class 'traction_plugins.multitenant_provider.v1_0.manager.AskarMultitokenMultitenantManager' not found.

@jamshale
Copy link
Contributor Author

@loneil Ok. Strange. I thought I updated all the deploy references. That error makes sense if the plugin config is still referencing that class. I'll try to find time to look.

@jamshale
Copy link
Contributor Author

jamshale commented Jan 12, 2024

@loneil I had some permissions problems in my repo and think it caused me to miss the one chart value. Should be fixed.

@loneil
Copy link
Contributor

loneil commented Jan 12, 2024

Ok cool. Yeah I've done regression on that PR going through Tenant/Innkeeper onboard, endorser connect, schema/cred creation, connect to BC Wallet, Issue, Revoke, Pres Req. So that's all working and that would be touching the plugins for sure.

The one weird thing though is that I can Basic Message back and forth from my Traction Tenant to the BC Wallet all good. But it's not fetching the messages from the basic message storage or something...?

So this is the aca-py agent logging out the received message I sent from my wallet
image

So can see the plugin getting it and putting it in storage.

But the GET is empty
image

Might try locally later on as well once I finish something else up, but not sure if any initial thoughts @jamshale

@jamshale
Copy link
Contributor Author

@loneil That is weird.

Oh. I forgot that @Jsyro had changed it recently to require a config that enables it. That might be the problem. I'll try and look at it on Monday. Thanks for the testing.

@esune
Copy link
Member

esune commented Jan 13, 2024

Changes look good to me, thanks @jamshale !

@loneil the basicmessage_storage now requires persistence to be enabled, it will by default NOT store messages - this was done to meet potential STRA/PIA requirements about data retention in our production environments.

@loneil
Copy link
Contributor

loneil commented Jan 13, 2024

Ohhh, I'd missed that. Would definitely make sense.

@Jsyro
Copy link
Contributor

Jsyro commented Jan 18, 2024

There should be logging that explains when it's being saved or not. but let me know if you need help figuring out the flag.

@jamshale
Copy link
Contributor Author

@loneil I've manually tested this and I'm pretty sure everything is working correctly and this can be merged.

esune
esune previously approved these changes Jan 22, 2024
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

@loneil
Copy link
Contributor

loneil commented Jan 22, 2024

If it is set up as the PR is for the DEV (and other) deployments, which it looks like it should be (?) then it's probably good to merge in to main and trigger the dev deployment.

So given that, I'd still want to record thoughts/questions about how to maintain Traction with this setup here. Some of these might have been previously discussed (maybe need documentation in the Traction repo?). Maybe worth discussion with @esune about if we have operational plans for this stuff, or priority for figuring out (if not already)

Running Traction locally with Docker

  • Still just docker compose build/up. Pulls from plugin repo for relevant plugins.

Local devcontainer in Traction repo

  • How does this work/is this needed?
  • Can we debug into plugins this way (that was possible before. though I never did it)

Local development for Innkeeper plugin

  • Should proceed the same as before
  • Innkeeper plugin remains in Traction repo

Local development for other plugins

  • Will involve changing local dockerfile to import files from local machine?
  • So you clone the plugin repo and then alter the dockerfile to point at those. Then change the plugins and rebuild local Traction? Then not commit the dockerfile?
  • This flow, what the best practices for this should be (should plugins be locally developed/debugged against Traction? Just an ACA-Py?) will need documentation

Updating ACA-Py version

  • Involves updating aca-py image in Traction, and then each plugin potentially needs to have ACA-Py Python package dependencies updated (aries-askar, indy-vdr, etc).
  • So this would be needing to poetry update the plugin repo and commit it before building Traction?
  • Would we only do plugins that Traction uses? Or are all plugins to be updated at the same time.
  • We usually want to test releases on OpenShift before going ahead, so would that involve the plugin repo needing to be moved forward so a PR env can be built on that linked codebase? Is there to be a different flow?

Updating other dependencies

  • Same sort of concept as ACA-Py version updates.
  • Security, dependabot, etc updates for poetry imports
  • Need to push to main before building a Traction?

@jamshale
Copy link
Contributor Author

I won't answer all the questions because there is a lot of them but the flow shouldn't change a lot. The plugin acapy versions are defined as extras so just upgrading the acapy image should be enough. The plugin repo has dev containers so the development env will be the same. You still needed to rebuild the plugin image in traction to get code changes. You can simply change the pip install location to your own fork when developing. The main change is you need to get a PR approved and merged into the plugin repo. This is actually a good thing because plugins need to have unit and integration tests which is lacking currently.

I'll do a development demo at some point, or if anyone starts work on a plugin and wants a run down. Some documentation in traction would probably be helpful in the longterm.

@loneil
Copy link
Contributor

loneil commented Jan 23, 2024

Yeah just wanted to record those thoughts somewhere for later. So yeah I think we can merge it and sort those as we go on.
Development demo sounds like a great idea. As well as adjusting development documentation in Traction.

@jamshale
Copy link
Contributor Author

Changed how the plugin config was defined to match the other plugins. Going to test again. If I find time I can add some documentation or I'll create another ticket for that.

@esune
Copy link
Member

esune commented Jan 23, 2024

I think we can merge, I agree with the above discussion/thread. If there is need for adjustment, we'll log an issue and tackle it that way - this is already beneficial as it will reduce code duplication.

@jamshale
Copy link
Contributor Author

@esune @loneil Ok I tested the recent config change. Working locally and the PR deploy.

I'll add a ticket for adding some documentation about developing remote plugins but don't have time to do it atm.

@esune
Copy link
Member

esune commented Jan 24, 2024

Logged #996 to track that work item, will now merge this.

@esune esune merged commit 799d44e into main Jan 24, 2024
7 checks passed
@esune esune deleted the use-plugin-repo branch January 24, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants