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

Feature/Redis Trigger #117

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Conversation

suneetnangia
Copy link
Contributor

@suneetnangia suneetnangia commented Jul 20, 2023

Added Redis trigger support in addition to existing http in the shim.
I'll add integration test support for this and also the existing "spin-outbound-redis" in next PR.

We are currently using a private version of this shim in our repo to demo wasm/containers messaging using Redis in K3d. https://github.com/suneetnangia/wasm-orchestration-with-spin

P.S. Let me know if you want to make any changes. Thanks

Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

LGTM! Made a few comments.

FYI, I am working on upgrading the spin shim to use youki container runtime, and in the process of doing that, I am thinking about using generic trigger from Spin so that it not only supports http and redis, but any spin custom triggers. But I do think this PR is mergable.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
containerd-shim-spin-v1/Cargo.toml Outdated Show resolved Hide resolved
info!(" >>> running spin trigger");
f = redis_trigger.run(spin_trigger::cli::NoArgs);
}
_ => todo!("Only Http and Redis triggers are currently supported."),
Copy link
Member

Choose a reason for hiding this comment

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

I am working on a generic trigger implementaion for the spin shim. This definitely helps!

@suneetnangia
Copy link
Contributor Author

All done @Mossaka. Thanks

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm!

Would you like to add the spin-inbound-redis image to the release pipeline? @suneetnangia

@Mossaka
Copy link
Member

Mossaka commented Jul 24, 2023

I am going to merge this in and we can have follow ups resolved in future PRs.

@Mossaka Mossaka merged commit 1cb6aaa into deislabs:main Jul 24, 2023
9 checks passed
@suneetnangia
Copy link
Contributor Author

lgtm!

Would you like to add the spin-inbound-redis image to the release pipeline? @suneetnangia

Can do, but was wondering if I should add integration tests for redis based images (both inbound and outbound (existing)) prior to release, to ensure images always work when we make changes?

@Mossaka
Copy link
Member

Mossaka commented Jul 29, 2023

@suneetnangia it's always a good idea to add more integration tests, specifically for redis based images.

Mossaka added a commit to Mossaka/shims that referenced this pull request Nov 13, 2023
This commit adds an integration test for spin-keyvalue using dynamic config file that was introduced by deislabs#179 and an integraion test for outbound redis introduced by deislabs#117

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Mossaka added a commit to Mossaka/shims that referenced this pull request Nov 14, 2023
This commit adds an integration test for spin-keyvalue using dynamic config file that was introduced by deislabs#179 and an integraion test for outbound redis introduced by deislabs#117

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Mossaka added a commit that referenced this pull request Nov 15, 2023
* test(spin): add more integration tests

This commit adds an integration test for spin-keyvalue using dynamic config file that was introduced by #179 and an integraion test for outbound redis introduced by #117

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
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.

2 participants