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

sip: Update SIP 005 #1753

merged 1 commit into from
Sep 18, 2023

Conversation

lann
Copy link
Collaborator

@lann lann commented Sep 11, 2023

No description provided.

Comment on lines -74 to -75
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:
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.

@@ -104,11 +104,11 @@ id = "hello"

## 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

Copy link
Collaborator

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

  • Would it be possible to move trigger information into the component trigger section?

  • I'm for removing base but could we call out why folks would want to use it? I'm still not super clear on why someone would want to use base over adding that to the path by hand with Spin.

@lann lann force-pushed the revamp-manifest-redesign branch from 3f1bd29 to a05618c Compare September 11, 2023 17:16
@lann
Copy link
Collaborator Author

lann commented Sep 11, 2023

I left out a bunch of changes that I meant to include, now added.

@lann
Copy link
Collaborator Author

lann commented Sep 11, 2023

I'm still not super clear on why someone would want to use base over adding that to the path by hand with Spin.

Me neither...


[[component]]
id = "world"
handler = "world"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be handler_component? Handler makes sense, because that's what it does :-), but below the proposal still refers to it as a component [component.world].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, it could be. Just a tradeoff between being concise and being explicit I guess 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes it easier to talk about if it's explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Opinion alert: I don't think there is a danger of confusion with just handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @radu-matei ....want to avoid component studder everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on that Brian? Are we trying to get rid of component as a Spin concept, and/or align with webassembly concepts and/or wasi-http concepts?

Copy link
Contributor

@fibonacci1729 fibonacci1729 Sep 13, 2023

Choose a reason for hiding this comment

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

I think what motivates my lean towards the concise here is that conceptually a trigger, as modeled in WIT (or generally as a component), might import an instance of whatever http component (hello in this example) and that import name expressed in WIT might look like import handler: ... or import fermyon:spin/handler. You probably wouldn't say in WIT import handler_component: ....

I definitely tend to prefer explicitness but in this case (albeit without great justification) I lean towards the concise I suppose. Seeing *_component throughout a larger example manifest would be hard on the eyes, me thinks.

This is not a hill I'm willing to die on though.

Alternative suggestion, what if we named the field simply component which would ameliorate the concise vs explicit tension (which sort of destroys my previous argument 🤣).

🤷‍♂️

TL;DR: lazy engineer want type little chars possible. 😋

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the input. Rereading the combined example, I can see how the 'handler' as part of a 'trigger' section, as a reference to 'who is handling this trigger' (which is a reference to a component; either inline source, or a 'component' section.) is easy enough to explain. Thx!

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:
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.

@lann lann force-pushed the revamp-manifest-redesign branch from a05618c to 106ee7d Compare September 12, 2023 20:23
Copy link
Contributor

@mikkelhegn mikkelhegn left a comment

Choose a reason for hiding this comment

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

LGTM

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

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@karthik2804
Copy link
Contributor

Does this issue discussion come into the scope of this PR as well?

@lann
Copy link
Collaborator Author

lann commented Sep 15, 2023

Does this issue discussion come into the scope of this PR as well?

Not necessarily: it looks like the proposal there would fit into either a v1 or v2 manifest in pretty much the same way.

Copy link
Collaborator

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

All of these changes look great. The only thing I'm a little confused about is generated IDs. Can you expand a bit more on that? What is generating these IDs? What does that experience look like? Why would you go for using generated IDs?

@lann
Copy link
Collaborator Author

lann commented Sep 18, 2023

generated IDs

Generated IDs would only apply to "inline" component defs like

[[trigger.http]]
route = "/hello"
handler = { path = "hello.wasm" }

In this case the trigger doesn't need an explicit component ID to reference, but we might still want the component to have an ID for things like #1116.

I think we can leave the exact generated IDs up to the implementation but I would imagine something like inline-1 or maybe something more clever like inline-hello.

@lann lann merged commit 1a40f09 into main Sep 18, 2023
@lann lann deleted the revamp-manifest-redesign branch September 18, 2023 21:30
@lann lann mentioned this pull request Oct 13, 2023
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.

7 participants