allow installing select templates #2238

karthik2804 · 2024-01-22T00:30:34Z

Adds the ability to be able to install specific templates using the following:

spin templates install --git https://github.com/fermyon/spin --id https-rust,http-go
...
Installed 2 template(s)

+-------------------------------------------------+
| Name        Description                         |
+=================================================+
| http-go     HTTP request handler using (Tiny)Go |
| http-rust   HTTP request handler using Rust     |
+-------------------------------------------------+

Skipped 10 template(s)

+-------------------------------------+
| Name                Reason skipped  |
+=====================================+
| http-c              Id not included |
| http-empty          Id not included |
| http-grain          Id not included |
| http-php            Id not included |
| http-swift          Id not included |
| http-zig            Id not included |
| redirect            Id not included |
| redis-go            Id not included |
| redis-rust          Id not included |
| static-fileserver   Id not included |
+-------------------------------------+

~~Todo: fix inconsistency between install and upgrade where upgrade always installs all the existing templates.~~ Currently upgrade installs all the templates it finds even if the user has manually deleted some templates. Is this the behavior we want or should it actually check if a template exists before upgrading it?

fixes #2235

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>

karthik2804 · 2024-01-22T00:38:10Z

crates/templates/src/manager.rs

+        if !install_template_list.is_empty() && !install_template_list.contains(&id.to_owned()) {
+            let message = format!("Skipping template {}...", id);
+            reporter.report(message);
+            return Ok(InstallationResult::Skipped(
+                id.to_owned(),
+                SkippedReason::IdNotIncluded,
+            ));
+        }


Should this be moved into its function? Feels wrong to be passing in a vec of IDs to the install_one method.

This is still installing one template though, so the name is no less correct. But yeah, it's a bit weird that we call it for templates we don't want to install...!

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>

karthik2804 · 2024-01-22T05:00:40Z

src/commands/templates.rs

+        let installed: Vec<String> = template_manager
+            .list()
+            .await?
+            .templates
+            .into_iter()
+            .map(|s| s.id().to_owned())
+            .collect();


Do we also want to filter based on the repo it is from?

itowlson · 2024-01-22T20:21:32Z

crates/templates/src/manager.rs

@@ -68,6 +68,8 @@ pub enum SkippedReason {
    AlreadyExists,
    /// The template was skipped because its manifest was missing or invalid.
    InvalidManifest(String),
+    /// The template name does match the provided IDs


Is there meant to be a 'not' here? Otherwise I'm unsure what this is saying.

Yep - that is a typo. will fix.

itowlson · 2024-01-22T20:25:05Z

crates/templates/src/manager.rs

@@ -181,6 +191,15 @@ impl TemplateManager {
        };
        let id = template.id();

+        if !install_template_list.is_empty() && !install_template_list.contains(&id.to_owned()) {
+            let message = format!("Skipping template {}...", id);


If the user has asked for a specific template, do they need to see skip messages for the other templates? (Currently you print the skipped ones in the list anyway I think?)

I agree and will remove it.

itowlson · 2024-01-22T20:30:17Z

crates/templates/src/manager.rs

+        if !install_template_list.is_empty() && !install_template_list.contains(&id.to_owned()) {
+            let message = format!("Skipping template {}...", id);
+            reporter.report(message);
+            return Ok(InstallationResult::Skipped(
+                id.to_owned(),
+                SkippedReason::IdNotIncluded,
+            ));
+        }


This is still installing one template though, so the name is no less correct. But yeah, it's a bit weird that we call it for templates we don't want to install...!

itowlson · 2024-01-22T20:32:51Z

crates/templates/src/manager.rs

@@ -181,6 +191,15 @@ impl TemplateManager {
        };
        let id = template.id();

+        if !install_template_list.is_empty() && !install_template_list.contains(&id.to_owned()) {


This makes the empty vector a magic value that means "all" rather than "none." Consider an enum with appropriate methods.

(One of those "the CLI / serialisation / etc. representation is not necessarily the correct parsed representation" situations.)

Good point. Will make the changes required.

itowlson · 2024-01-22T20:37:11Z

crates/templates/src/manager.rs

@@ -121,6 +123,7 @@ impl TemplateManager {
        source: &TemplateSource,
        options: &InstallOptions,
        reporter: &impl ProgressReporter,
+        install_template_list: &Vec<String>,


Is there a reason for this to be a separate parameter rather than part of InstallIOptions?

Nothing more than me just hacking something quickly, I will move it as a part of the InstallOptions.

itowlson · 2024-01-22T20:40:09Z

src/commands/templates.rs

+
+    /// By default, Spin upgrades all the templates in the source.
+    /// Pass this flag with a list of template IDs to update specific ones.
+    #[clap(long, value_delimiter = ',')]


I don't love the comma separator - I think we usually use allow-multiple args for this sort of thing? (But I may be wrong!)

I do not have a really strong preference but having to type --id template-a --id template-b seemed a bit more verbose but that does seem more semantically correct with it being id (singular)

itowlson · 2024-01-22T20:41:47Z

src/commands/templates.rs

        if self.git.is_some() {
            // This is equivalent to `install --update`
            let install = Install {
                git: self.git.clone(),
                branch: self.branch.clone(),
                dir: None,
                update: true,
+                id: installed,


Doesn't this mean that if we add a new template to the Spin repo, existing users will never get it? That seems regrettable.

Yes but the issue that I see with the current approach is that, If I install all the templates from Spin repo and then decide I do not want to write zig or c code and delete the templates and then upgrade, I end up getting all the templates installed again.

Yes but deleting C doesn't mean I want to miss out on Befunge when I've been waiting so long for it. Worse still, with this, even if I never deleted anything I will still never get Befunge, because this implementation locks me in to my current set of templates.

I'm fully in favour of respecting deletes or "I only ever asked for http-rust" across an upgrade - but we must have an answer for that first. We need to write down the desired (and undesired) behaviours, and then design something that will behave correctly, and I fear that will have to be rather more complicated and nuanced than this simple initial approach (which I realise is a draft for feedback, but this is the feedback grin).

I agree with this needing discussion. My take on this is that upgrade only upgrades what I have. If I need something new that I have never had I would have to install first. I can definitely be convinced otherwise.

I am happy to remove the changes to the upgrade part and leave the behavior as it is today if that is a preferred approach.

Yes, the trouble is that historically what I have had is a repo rather than a set of individual templates. (For better or for worse!)

As for what to do in this PR... the thing is, I agree that if someone has said "I only want http-rust" then an upgrade should only give them http-rust. The current behaviour is definitely not the preferred approach! So I am not sure that we can punt on this unfortunately...

I have given it a little more thought and to me, the more I am convinced upgrade should not bring in anything new. Even though it has historically done so, I think using the command spin templates install --update ... is what I seem to recall in most places so this should not have too much impact potentially (?).

itowlson · 2024-01-22T20:46:49Z

src/commands/templates.rs

@@ -570,6 +583,7 @@ fn skipped_reason_text(reason: &SkippedReason) -> String {
    match reason {
        SkippedReason::AlreadyExists => "Already exists".to_owned(),
        SkippedReason::InvalidManifest(msg) => format!("Template load error: {}", msg),
+        SkippedReason::IdNotIncluded => "ID not included".to_owned(),


I'm a bit torn on this. Part of me wants to not print it because it wasn't within the scope of what the user asked to install. But another part of me notes that if the user typoes an ID then it will help them find what it was meant to be.

In any event if we do print it I'd suggest a message such as "Not requested" that reflects user intent (I just want the C template) rather than the implementation (I passed a list of IDs).

I think we can have this table instead of the reporter messages with the reason "Not requested" as suggested.

itowlson · 2024-01-22T20:52:32Z

crates/templates/src/manager.rs

@@ -181,6 +191,15 @@ impl TemplateManager {
        };
        let id = template.id();

+        if !install_template_list.is_empty() && !install_template_list.contains(&id.to_owned()) {


The problem with a pure filtering approach is error handling. If I write install --id http-rsut it will cheerily report "No templates installed" and the user is going "but where is my Rust template." The user is unlikely to think of their command as filtering: they think they have asked for a specific template. So if we can't find the template they asked for, that feels like we should be telling them "hey we didn't find http-rsut" and maybe even "but may we interest you in this charming http-rust, very popular, very in right now."

I agree that the preferred approach should be to let them know if we did not find a template that they asked for.

karthik2804 · 2024-03-11T18:13:36Z

I am closing this as it will require more work to be put into the redesign of the current template system.

karthik2804 requested a review from itowlson January 22, 2024 00:30

allow installing select templates

330551e

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>

karthik2804 force-pushed the install_specific_templates branch from 24b2b5f to 330551e Compare January 22, 2024 00:36

karthik2804 commented Jan 22, 2024

View reviewed changes

update only existing templates

865bd92

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>

karthik2804 commented Jan 22, 2024

View reviewed changes

itowlson requested changes Jan 22, 2024

View reviewed changes

karthik2804 closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow installing select templates #2238

allow installing select templates #2238

karthik2804 commented Jan 22, 2024 •

edited

Loading

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 23, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

itowlson Jan 22, 2024

karthik2804 Jan 22, 2024

karthik2804 commented Mar 11, 2024

allow installing select templates #2238

allow installing select templates #2238

Conversation

karthik2804 commented Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthik2804 commented Mar 11, 2024

karthik2804 commented Jan 22, 2024 •

edited

Loading