remove single trigger compatibility validation#3629
Conversation
| // only manifests subject to a compatibility check. v2 manifests may host | ||
| // multiple trigger types. Triggerless templates also allowed for all | ||
| // manifest versions. | ||
| trigger_types: Option<Vec<String>>, |
There was a problem hiding this comment.
I think we can leave this as Option<String>, because if it's v1, it can only be a single type, and if it's v2, we don't care about the type anyway.
But however we do it we need some way to distinguish between "this is a v1 with this trigger type" and "this is a v2 which currently has only one trigger type."
Although... really, at this point, our templates can't add to a v1 manifest anyway, because the trigger and component syntax is all wrong. So perhaps what we should do is:
- detect if we are adding to a v1 manifest
- if so, fail gracefully
- remove everything to do with trigger types and trigger type checking
It wouldn't be a breaking change because it's ahem already broken...
There was a problem hiding this comment.
That makes sense to me. I reworked this PR to reflect that flow. Lmk what you think.
| fn infer_trigger_type(layout: &TemplateLayout) -> TemplateTriggerCompatibility { | ||
| match crate::app_info::AppInfo::from_layout(layout) { | ||
| Some(Ok(app_info)) => match app_info.trigger_type() { | ||
| Some(Ok(app_info)) => match app_info.trigger_types().and_then(|types| types.first()) { |
There was a problem hiding this comment.
With the current implementation this feels like it would constrain a v2 application to the first trigger type in its manifest, but maybe I'm misreading it?
(But per comment above I think we should ditch all checking anyway, and simply fail on v1 manifests.)
There was a problem hiding this comment.
yeah so if you trace the function call, infer_trigger_type is only being used for v1 manifests but your suggestion on ditching the check altogether makes sense to me.
| } | ||
|
|
||
| #[test] | ||
| fn can_read_app_info_from_multi_trigger_template_v2() { |
There was a problem hiding this comment.
We probably want to test somewhere that we can add a new trigger type to an existing (single-trigger-type) manifest.
473af83 to
88cb690
Compare
+ resolves spinframework#3626 Signed-off-by: Michelle Noorali <michelle.noorali@fermyon.com>
88cb690 to
14fdb24
Compare
+ removes trigger validation as it is no needed + spin add for manifest v1 no longer supported Signed-off-by: Michelle Noorali <michelle.noorali@fermyon.com>
14fdb24 to
043d82a
Compare
2 participants
spin add: remove trigger compatibility check #3626