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

Add redis-pq-demo #78

Merged
merged 20 commits into from
Sep 8, 2023
Merged

Conversation

Jsyro
Copy link
Contributor

@Jsyro Jsyro commented May 18, 2023

Add a configuration that leverages the redis-persistent-queue plugin.

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

Great work putting this together- already used in rudimentary load testing.

BIggest comment is for documentation. If I handed that readme to a green dev and asked them to stand all of it up, they couldn't. And then not without a lot of questions. We should be providing as much context and as many commands for someone to get up and running without jumping through too many hoops. The fewer things they need to figure out and troubleshoot, the better.

redis-pq-demo/docker-compose.redis.yml Outdated Show resolved Hide resolved
redis-pq-demo/docker-compose.redis.yml Show resolved Hide resolved
redis-pq-demo/docker-compose.yml Show resolved Hide resolved
--admin 0.0.0.0 ${MEDIATOR_AGENT_HTTP_ADMIN_PORT}
--admin-api-key ${MEDIATOR_AGENT_ADMIN_API_KEY}
--endpoint ${MEDIATOR_URL_E} wss://${MEDIATOR_URL_E}
--plugin redis_queue.v1_0.events
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is a discussion for acapy + plugins, but the way this is built and then referenced does not match up with the documentation in that redis-queue plugin. there it will say that --plugin redis_queue.v1_0. This works and is great but just pointing out that we need to figure out why it is like this here and if that will be the standard going forward. Really great that you got that image built with the plugin and working - super useful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the redis_queue plugin has 3 components, so I'm wondering if this is specifically referencing the events component of that plugin, but ignoring the other two.

I'm not sure how this would differ from installing the other two. I'm fairly certain we want the events component and not the inbound (relay) or outbound (deliverer) ones.

redis-pq-demo/requirements.txt Outdated Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/.env.sample Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/README.md Outdated Show resolved Hide resolved
redis-pq-demo/README.md Show resolved Hide resolved
redis-pq-demo/README.md Show resolved Hide resolved

mediator:
build:
context: .
Copy link
Contributor

Choose a reason for hiding this comment

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

The build for this works (as in it stands up a mediator with redis_queu), but doesn't use ngrok even if you want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working to reinstate ngrok easily. I agree this should be part of the demo.

@jleach
Copy link
Contributor

jleach commented Jul 17, 2023

@Jsyro @usingtechnology Checking in on this one. Still a thing?

@usingtechnology
Copy link
Contributor

I'll have to look at it again to see if it addressed the issues/concerns I raised. I'll try to carve out some time tomorrow. @Jsyro can comment on whether he had work outstanding on this.

@WadeBarnes
Copy link
Contributor

Quick reminder this PR needs to be reviewed so we can update some mediator deployments and get them off the development fork.

@usingtechnology
Copy link
Contributor

Follow up - I did check it on Jul 18 and seemed good, was waiting on @Jsyro to chime in on if his work was complete... didn't hear so fell off my radar. My concerns are alleviated.

I believe he is testing it out today to make sure it works alongside other changes done in the meantime.

@esune
Copy link
Member

esune commented Sep 7, 2023

@Jsyro please chime-in as soon as possible on whether we can go ahead and merge/deploy this or further changes are needed.

@Jsyro
Copy link
Contributor Author

Jsyro commented Sep 7, 2023

I reused my README and had one issue regarding which dockerfile to use. I also had to modify the scripts to put all containers on the same network for local testing because ngrok is blocked by my corporate policies.

The configuration properly used redis and it can be inspected by the redis ui tool.

@usingtechnology
Copy link
Contributor

i suppose i can test it out as is (with ngrok). are you documenting the steps to take if you cannot use ngrok?

@usingtechnology
Copy link
Contributor

Dockerfile.ngrok has FROM mediator_pq_acapy. mediator_pq_acapy is never built anywhere.

Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
Signed-off-by: Jason Syrotuck <[email protected]>
@Jsyro
Copy link
Contributor Author

Jsyro commented Sep 7, 2023

The demo steps are cumbersome, but do work as described. Requiring the tester to have python and pip and install dependencies should be looked at in the future. A Simpler example should be fairly easy to make.

@esune
Copy link
Member

esune commented Sep 7, 2023

I just ran the tests with @Jsyro and everything seems to work as expected.

@jleach are yuo able to review/approve/merge this one as well?

Signed-off-by: Jason Syrotuck <[email protected]>
@usingtechnology
Copy link
Contributor

The demo steps are cumbersome, but do work as described. Requiring the tester to have python and pip and install dependencies should be looked at in the future. A Simpler example should be fairly easy to make.

I was just about to add a ticket to simplify that. It wasn't written for this purpose so you could just swipe it and put it in this repo as an example (use env var or something for the mediator_url, use random ascii or uuid for the names instead of random-word).

perhaps that is better... move the sample in here? seems fragile to expect that script to remain in aca-py.

anyway, since you added in the redis-ui, you should have a quick blurb about where it is, how to connect and what they should see.

@WadeBarnes
Copy link
Contributor

@esune, @usingtechnology, @Jsyro, Let me know when this is ready to go. I can approve and merge.

jleach
jleach previously approved these changes Sep 8, 2023
acapy_default:
ipv4_address: 172.28.0.107
depends_on:
- redis-node-1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this allot a nodes for a local / docker instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe a redi cluster requires a minimum of 6 nodes.

Signed-off-by: Jason Syrotuck <[email protected]>
@Jsyro
Copy link
Contributor Author

Jsyro commented Sep 8, 2023

I believe i've addressed the last concern by @usingtechnology about what the redis-ui is there for.

I think this is ready for a final review/merge.

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

plugin and plugin config are done in two places (yml file and start.sh) - can we limit it to a single location?

also, there are files (local.yml, mediator-with-plugin.yml, provision.yml) that don't appear to be used, can we remove them?

Signed-off-by: Jason Syrotuck <[email protected]>
@Jsyro
Copy link
Contributor Author

Jsyro commented Sep 8, 2023

@usingtechnology Good catch. Fixed

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

👍 👍

@WadeBarnes WadeBarnes merged commit f6ed271 into openwallet-foundation:main Sep 8, 2023
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.

5 participants