-
Notifications
You must be signed in to change notification settings - Fork 251
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
Use spin.alt
domain for service chaining
#2736
base: main
Are you sure you want to change the base?
Conversation
crates/locked-app/src/locked.rs
Outdated
@@ -15,7 +15,7 @@ use crate::{ | |||
pub type LockedMap<T> = std::collections::BTreeMap<String, T>; | |||
|
|||
/// If present and required in `host_requirements`, the host must support | |||
/// local service chaining (*.spin.internal) or reject the app. | |||
/// local service chaining (*.spin.alt/.internal) or reject the app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Only reading the doc comment it's somewhat unclear whether both are supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, that also makes me think that this becomes another compatibility flag because an app that uses spin.alt
won't work in an environment that supports only spin.internal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we're doing this with a major release I think it should be fine to just note in the docs that/how it changed for 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That rather flies in the face of being able to validate that an application runs in other environments though, which was the whole point of adding the host requirements to the lockfile in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the principled thing would be to add a e.g. local_service_chaining_alt
host requirement.
Just going to note that we'll want to bring this change into the Factors work as well. We're close to merging that into main which will get rid of the possibility of things landing in main but not in factors. |
Signed-off-by: itowlson <[email protected]>
5884cfc
to
fe31cfa
Compare
Fixes #2731