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

sip: Update SIP 005 #1753

Merged
merged 1 commit into from
Sep 18, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 63 additions & 36 deletions docs/content/sips/005-manifest-redesign.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ date = "2022-05-20T13:22:30Z"

Summary: A new design for Application Manifests (`spin.toml`)

Owner: lann.martin@fermyon.com
Owner: <lann.martin@fermyon.com>

Created: May 20, 2022
Updated: Sep 11, 2023

## Background

Expand All @@ -16,14 +17,15 @@ components, and trigger configurations. The current design supports a single tri
component and trigger configuration with an implicit one-to-one relationship:

```toml
spin_manifest_version = "1"
name = "hello-world"
trigger = { type = "http", base = "/hello" }

[[component]]
id = "world"
id = "hello"
# ...
[component.trigger]
route = "/world"
route = "/hello"
```

In order to support future features, we would like to enable more flexible trigger configurations:
Expand All @@ -37,6 +39,18 @@ In order to support future features, we would like to enable more flexible trigg

## Proposal

### Manifest version

Since this proposal represents a backward-incompatible change to the manifest format, the manifest
version changes.

> Note that this also changes the version from a string to an int, which is a minor/optional
> change for this proposal.

```toml
spin_manifest_version = 2
```

### Global trigger config

In order to allow for multiple trigger types, we remove the top-level `trigger.type` field.
Expand All @@ -53,39 +67,65 @@ base = "/hello"
> In the short term we can continue to enforce the use of exactly one trigger type in an
> application as a manifest validation step.

### Component config

Component IDs will become more important under this design, which we can emphasize
by moving the ID into the section header (aka TOML table key):

```toml
[component.hello-world]
# instead of id = "hello-world"
```

### Trigger config

In order to decouple triggers and components, we move triggers to new top-level
`[[trigger.<type>]]` sections:

```toml
[[trigger.http]]
route = "/world"
component = "world"

[[component]]
id = "world"
route = "/hello"
handler = "hello"
[component.hello]
# ...
```

### Manifest version
### Component inline config

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:
Comment on lines -74 to -75
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already renamed this key, separately.

In addition to being defined in their own top-level sections and referenced by ID,
component configs may be inlined; allowing this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the proposal want to allow this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It keeps the manifest smaller and linked component/trigger configs visually close together.


```toml
spin_manifest_version = "2"
[[trigger.http]]
route = "/hello"
handler = { source = "hello.wasm" }
```

which would be equivalent to:

```toml
[[trigger.http]]
route = "/hello"
handler = "<generated-id>"
[component.<generated-id>]
source = "hello.wasm"
```

### Rename `config`
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉 +1000


The Spin `config` feature is easily confused with Spin "Runtime Config". We can adopt
the `variables` terminology which is already used by the top-level `[variables]` section
and rename `[component.config]` to `[component.variables]`.

## Backward compatibility

Existing "version 1" manifests can be transformed into this "version 2" data structure losslessly:
Existing "version 1" manifests can be transformed into this "version 2" format losslessly:

```toml
trigger = { type = "http", base = "/" }
[[component]]
id = "hello"
source = "hello.wasm"
[component.trigger]
route = "/hello"
```
Expand All @@ -97,30 +137,17 @@ is equivalent to:
base = "/"
[[trigger.http]]
route = "/hello"
component = "hello"
[[component]]
id = "hello"
handler = "hello"
[component.hello]
source = "hello.wasm"
```

## Design options

### Don't update the manifest version
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this is actually a viable option.

Copy link
Member

Choose a reason for hiding this comment

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

+1


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
would be easy to detect if we switch the `spin_version` key.
This upgrade could be performed automatically by `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:
## Design options

```toml
[[trigger.http]]
id = "world"
```
### Remove http `base`

Mandatory IDs would require a translation from the current "v1" manifest, which could either
simply copy the component ID (if we're fine with component and trigger IDs being separate
namespaces), or e.g. add a suffix to the component ID (`world` -> `world-trigger`), which
could technically cause conflicts but doesn't seem likely in practice.
This trigger option is rarely used and has caused implementation headaches.
For any (rare) existing applications that do use it, the `spin doctor` upgrade
path could inline the `base` into the trigger `route`(s).