Fix redis/http triggers #800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I can confirm the redis-rust trigger template works with a build from this PR. |
etehtsea
force-pushed
the
fix-trigger
branch
4 times, most recently
from
8a98bcd
to
4b3d985
Compare
Refs: fermyon#763. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Fixes the following error:
$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger
Caused by:
metadata error: missing required "redis_address"
Error: exit status: 1
Refs: fermyon#763.
Closes fermyon#799.
Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Set base path in spin-http rather than joining it as part of the route. Split test configs in spin-testing because the trigger's configuration is different for http and redis. Refs: fermyon#763. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Example:
2022-10-03T09:04:25.628294Z ERROR spin_trigger::cli: Trigger executor failed
Error: Redis trigger failed to connect to redis://localhost:6379
Caused by:
Connection refused (os error 111)
Error: exit status: 1
Closes fermyon#704.
Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
|
Thanks. This must have been a bad rebase conflict resolution from right before merging. |
lann
approved these changes
Oct 3, 2022
Comment on lines
-53
to
+61
| .require_metadata("redis_address") | ||
| .context("Failed to configure Redis trigger")?; | ||
| .require_metadata::<TriggerMetadata>("trigger")? | ||
| .address; |
There was a problem hiding this comment.
It's only been three weeks and I can't even remember switching back to trigger here, let alone why. 🤔
This fix looks correct so I think its good to merge, but I'll probably be reverting this and fixing the matching metadata generation later (which should be fine as the metadata is an unstable interface).
| @@ -37,13 +37,18 @@ macro_rules! from_json { | |||
| } | |||
|
|
|||
| #[derive(Default)] | |||
| pub struct TestConfig { | |||
| pub struct HttpTestConfig { | |||
| trigger_type = "http"; | ||
| let route = base.trim_end_matches('/').to_string() + &route; |
There was a problem hiding this comment.
Ugh, missed this in a meta-refactor of the refactor PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore: Drop trigger-new leftovers
If this is intentional, I'll drop this commit.
fix(spin-redis-engine): Fix error during startup
Steps to reproduce:
Closes #799.
chore(spin-http): Fix base mapping
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.
The latter isn't a user-facing bug, but the
baseis set internally to/(because there is no such key in metadata) and all routes have abaseprefix in their paths. This commit makes the behavior consistent with the redis trigger.Also, out of the current testing crate, it looks like it's possible to have different triggers (
httpandredis) simultaneously in one spin app. According to the current templates, I couldn't figure out how to do this, so I split testing configs for now.