SIP: Application Manifest Redesign #516
Conversation
lann
force-pushed
the
manifest-redesign-sip
branch
3 times, most recently
from
b32cc08
to
88871f9
Compare
| Since this proposal represents a backward-incompatible change to the manifest format, the manifest | ||
| version changes. The existing `spin_version = "1"` field name leaves some room for misinterpretation | ||
| as "compatible with Spin project version 1". As an optional part of this proposal, change that name: | ||
|
|
||
| ```toml | ||
| spin_manifest_version = "2" | ||
| ``` |
There was a problem hiding this comment.
Per discussion with @fibonacci1729 we don't have to do this. See "Don't update the manifest version" below.
There was a problem hiding this comment.
I'm in favor of renaming spin_version to spin_manifest_version but I think we should wait to officially freeze version 1 manifest until spin 1.0
There was a problem hiding this comment.
Agree on this from an aesthetic point of view, but without some sort of schema ID, a user who downloads a new Spin build that changes the schema is going to get cryptic parse errors working with existing manifests. Can we make sure that we do at least enough to give a meaningful error like "this version of the manifest is no longer supported"?
There was a problem hiding this comment.
Says the man who just changed templates.
| > In the short term we can continue to enforce the use of exactly one trigger type in an | ||
| > application as a manifest validation step. | ||
|
|
||
| ### Trigger config |
There was a problem hiding this comment.
how will manifest define triggers that are outside of the spin repo?
There was a problem hiding this comment.
They are identified by string type, which maps to a binary spin-trigger-<type>.
| In order to support future features, we would like to enable more flexible trigger configurations: | ||
|
|
||
| - Multiple trigger types in an application | ||
| - Multiple triggers associated with a single component |
There was a problem hiding this comment.
Isn't the difference of trigger function signature prevents one component associating with multiple triggers?
For example, a Redis trigger and a HTTP trigger can't associae with a single component since their require linking to different .wit files.
There was a problem hiding this comment.
A component can implement multiple interfaces.
There was a problem hiding this comment.
Theoretically, you can do that today if you implement and export more than one interface (both the HTTP and the Redis entrypoint) and use the same .wasm file.
There was a problem hiding this comment.
I like this! The compatibility story is good, which minimises the number of people who will go "eh, too hard" and stop kicking the tyres, and I like the improved extensibility and cleaner organisation. A few comments/gripes but I definitely like the direction!
| ### Trigger IDs | ||
|
|
||
| We might want to be able to reference individual triggers, for example to implement a | ||
| `spin up --trigger=world`. It would save some compatibility headache if we require IDs now: |
There was a problem hiding this comment.
This feels a bit intrusive in the simple case (which is likely to be common especially amongst the vital tyre-kicker demographic). Could we, say, plan to default the ID to the trigger type, and require IDs only if there are multiple triggers of the same type?
There was a problem hiding this comment.
Oh wait is this meant to refer to individual components under a trigger? (or should I say trigger-config-to-component maps) This is what the example syntax seems to show, but if so I fear we may have two meanings for the term 'trigger'.
There was a problem hiding this comment.
I think my "trigger" is equivalent to your "trigger config". Each trigger has a "type", and a "trigger executor" can serve multiple triggers of a given type.
There was a problem hiding this comment.
So, in the HTTP case, each route is considered a separate trigger? (And for Redis it would be each channel, etc.?)
There was a problem hiding this comment.
Yep, and a single component may handle multiple triggers.
There was a problem hiding this comment.
This seems really useful especially for testing purposes but it would be really nice if we could make this optional so we can make "the mean time to dopamine" to understand a spin.toml file and spin up as low as possible.
There was a problem hiding this comment.
Yeah that's why this idea is down here in "Design options" and not up there 👆 in the proposal. 🙂
| ### Don't update the manifest version | ||
|
|
||
| Instead of updating the manifest version we could not do that. It would be nice to include | ||
| error message guidance and/or a `spin doctor` tool to upgrade the manifest format. This |
There was a problem hiding this comment.
I like this - super helpful!
There was a problem hiding this comment.
+1 ... I'm such a sucker for cute names like "spin doctor"
| ```toml | ||
| [application] | ||
| name = "hello-world" | ||
| [application.trigger.http] |
There was a problem hiding this comment.
This is a table rather than a table array, which means that (at some point) we can have multiple triggers of different types, but not multiple triggers of the same type, is that right? Or do we envisage revving the version at that point?
(Of course I might just be misunderstanding my TOML syntax here!)
There was a problem hiding this comment.
This is "application-wide HTTP trigger config". I'm not entirely happy with the syntax, but didn't come up with anything better.
| `[[trigger.<type>]]` sections: | ||
|
|
||
| ```toml | ||
| [[trigger.http]] |
There was a problem hiding this comment.
I'm not sure I see the value of this over putting the trigger-related config under the component. It adds a layer of indirection which means:
- Two places to edit the component ID
- When I look at the component it's not obvious what it's hooked up to (e.g. HTTP route)
There was a problem hiding this comment.
A couple of motivations:
- I think it makes more sense conceptually for triggers to be a top-level entity. Another design I considered was just inverting the component/trigger relationship, i.e. turning
component.triggerintotrigger.component, but: - I want to enable HTTP middleware (and other kinds of trigger-component relationships), where a single trigger may reference multiple components (one "handler" and zero+ "interceptors"). I like the consistency of referencing them all by ID.
It's true that it introduces some duplication and potential non-locality, but in practice you can group a trigger and its "main" component together in the file, e.g.:
[[trigger.http]]
route = "/hello"
component = "world"
[[component]]
id = "world"There was a problem hiding this comment.
@itowlson's perspective resonates with me because I have been thinking about Spin as a framework for building components and so components are the central subject in my head and everything revolves around them. However, when I tell myself that the spin.toml's job is to wire up triggers to components, then making the triggers a top level entity makes much more sense to me.
| `[[trigger.<type>]]` sections: | ||
|
|
||
| ```toml | ||
| [[trigger.http]] |
There was a problem hiding this comment.
@itowlson's perspective resonates with me because I have been thinking about Spin as a framework for building components and so components are the central subject in my head and everything revolves around them. However, when I tell myself that the spin.toml's job is to wire up triggers to components, then making the triggers a top level entity makes much more sense to me.
|
|
||
| ```toml | ||
| [application.trigger.http] | ||
| base = "/" |
There was a problem hiding this comment.
Can this top level application.trigger.http be optional/omitted if folks don't need to change base = "/"? The concept of having a base is a little distracting anyway when first looking at a spin.toml file.
There was a problem hiding this comment.
Yes to omitting it if it doesn't need to set anything. I suspect we could drop base entirely, in favor of custom config e.g. route = "{{base}}/route", but I didn't want to drop the "global trigger config" entirely.
| ### Don't update the manifest version | ||
|
|
||
| Instead of updating the manifest version we could not do that. It would be nice to include | ||
| error message guidance and/or a `spin doctor` tool to upgrade the manifest format. This |
There was a problem hiding this comment.
+1 ... I'm such a sucker for cute names like "spin doctor"
| ### Trigger IDs | ||
|
|
||
| We might want to be able to reference individual triggers, for example to implement a | ||
| `spin up --trigger=world`. It would save some compatibility headache if we require IDs now: |
There was a problem hiding this comment.
This seems really useful especially for testing purposes but it would be really nice if we could make this optional so we can make "the mean time to dopamine" to understand a spin.toml file and spin up as low as possible.
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
lann
force-pushed
the
manifest-redesign-sip
branch
from
88871f9
to
f280ae8
Compare
Rendered