spin add should check existing targets for templates#3512
Conversation
kate-goldenring
left a comment
kate-goldenring
left a comment
There was a problem hiding this comment.
Just a couple clarifying questions. Should any tests be updated/added?
| _ => { | ||
| terminal::warn!("This application targets multiple environments."); | ||
| eprintln!( | ||
| "Spin doesn't currently support environment-specific templates for multiple environments." | ||
| ); | ||
| eprintln!("Templates will instead be taken from the installed template set."); |
There was a problem hiding this comment.
What is the reason that we cannot support spin add for spin apps with multiple targets. Is the idea that it is impossible to take the intersection of two templates directories? This almost suggests that each template should have a universal id. I can see many users adding Spin CLI and SpinKube envs by default to most apps.
There was a problem hiding this comment.
See the four-ish messages beginning at #3494 (comment). It's not a great situation I admit.
| ); | ||
| } | ||
| TemplateVariantInfo::AddComponent { .. } => { | ||
| // `spin add` doesn't have an option for skipping a templateless env (the equivalent of `spin new` without `-E`). |
There was a problem hiding this comment.
This comment seems to suggest there is a todo or something to be done here. But it seems expected that spin add infers E from the spin.toml. My only concern here is that the eprintln seems to indicate it is errorsome for a target env to not provide templates. is that the case?
There was a problem hiding this comment.
It's not errorsome, but it does mean we can no longer guarantee in advance that the templates are vetted to be suitable for the given TE. Like if you targeted Spin 3.6, and that didn't declare any templates, you'd get the Spin 4 templates, which might land you with WASI P3. Hence the warning. I'm happy to reword it.
(That said, my feeling is leaning to "a TE probably should include templates because otherwise it's saying that some random version of the Spin templates will work with it.")
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
add-respect-target-env-templates
branch
from
f106ee5 to
f9cdce7
Compare
3 participants
If an existing application declares exactly one target environment, and that target environment declares templates,
spin addshould use those templates.This turned out to not play very nicely with the existing flow. I've gone with it for now, but this may be pointing us to want to rework some of the
new/addstuff more substantially - some of the functions are getting a bit long and having to check a lot of cases. But I don't think we need to tackle this now.