`spin doctor` prototype #1435

lann · 2023-04-28T19:39:52Z

The subcommand is currently hidden from e.g. spin --help.

$ spin doctor -f examples/http-rust/spin.toml
📟 The Spin Doctor is in.
🩺 Checking examples/http-rust/spin.toml...

❗ Diagnosis: Manifest 'spin_manifest_version' must be "1", not 1
🩹 The Spin Doctor can help! Would you like to:
  Set manifest version to "1"
  Do nothing
> See more details about the recommended changes

📋 Apply the following diff to "examples/http-rust/spin.toml":
@@ -1,2 +1,2 @@
-spin_manifest_version = 1
+spin_manifest_version = "1"
 authors = ["Fermyon Engineering <engineering@fermyon.com>"]

Would you like to apply this fix? [Y/n] 
❤️ Treatment applied!

calebschoepp

Couple questions but otherwise very excited about this, LGTM

Cargo.toml

crates/doctor/Cargo.toml

src/commands/build.rs

src/commands/doctor.rs

crates/doctor/src/lib.rs

crates/doctor/src/manifest/trigger.rs

itowlson

A number of nits and suggestions, most if not all of which can be deferred to future PRs; really good stuff and I really appreciated the clean structure of it.

crates/doctor/src/lib.rs

itowlson · 2023-05-02T03:53:46Z

src/commands/doctor.rs

+
+impl DoctorCommand {
+    pub async fn run(self) -> Result<()> {
+        eprintln!("{icon}The Spin Doctor is in.", icon = Emoji("📟 ", ""));


Well, this is another matter of taste... I really dislike peppering command output with emoticons, because the effect is often this kind of rainbow soup of mismatched shapes and colours where nothing stands out any more (and of course console icons tend not to line up with OS conventions), and so becomes visual clutter rather than guiding the eye to different stanzas and states. I think they can be super useful when used to support and structure the UI, though, so we can iterate on this for sure.

To be even nittier (and more pet peevish), this goes double for relatively detailed icons like the "checking" icon (stethoscope?) - there is quite a bit of detail below text size and my old eyes find that difficult. If we are having icons then let's choose ones that are relatively bold, simple shapes.

I expected this to be controversial. 😉 😛 🐱

I'm not married to the emoji per se but I do think the UX benefits from some visual cues to break up the chunks of interaction.

To throw my opinion in the ring: I quite like emoji because I do find that it helps guide my eye, but more than anything I would prefer color output. I find spin output generally very hard to read because it all comes out looking like a wall of text.

src/commands/doctor.rs

crates/doctor/src/test.rs

crates/doctor/src/wasm/missing.rs

src/commands/doctor.rs

itowlson · 2023-05-02T04:56:15Z

src/commands/doctor.rs

+#[clap(hide = true, about = "Run the Spin Doctor")]
+pub struct DoctorCommand {
+    #[clap(short = 'f', long, default_value = "spin.toml")]
+    file: PathBuf,


Future suggestion: a --dry-run or --check flag that prints the diagnoses without any interaction and returns distinctive exit codes for "no diagnoses", "non-critical only", and "critical, allowing it to be used as a linter?

Ooh I really like this

src/commands/doctor.rs

rylev · 2023-05-02T14:25:57Z

crates/doctor/src/lib.rs

+
+use std::{fmt::Debug, fs, future::Future, path::PathBuf, pin::Pin, process::Command, sync::Arc};
+
+use anyhow::{ensure, Context, Result};


nit: I know we do this everywhere, but I really prefer not using Result unqualified. Unqualified Result should always be std::result::Result. Everywhere else we should use it qualified (e.g., anyhow::Result, io::Result, etc.). You don't need to address that here though since this is far from the only place where this occurs.

While I understand the rationale, I don't think it's necessary for anyhow::Result because 1) it is ubiquitous across the codebase (for better or worse) and 2) its definition makes it compatible with std::result::Result:

pub type Result<T, E = Error> = core::result::Result<T, E>;

crates/doctor/src/lib.rs

crates/doctor/src/manifest/trigger.rs

rylev · 2023-05-02T14:39:05Z

src/commands/doctor.rs

+
+impl DoctorCommand {
+    pub async fn run(self) -> Result<()> {
+        eprintln!("{icon}The Spin Doctor is in.", icon = Emoji("📟 ", ""));


To throw my opinion in the ring: I quite like emoji because I do find that it helps guide my eye, but more than anything I would prefer color output. I find spin output generally very hard to read because it all comes out looking like a wall of text.

src/commands/doctor.rs

crates/common/src/spin_env.rs

src/commands/doctor.rs

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

While "old version key" and "wrong value" are logically different issues, from a UX perspective it seems better to let the "more critical" "wrong value" issue take precedence. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

This changes the way diagnoses are "registered" with a Checkup; rather than storing function pointers it now stores trait objects. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

The former is a short description to be displayed as a choice while the latter is a longer e.g. diff of what the treatment will do. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

itowlson

I suggest we land this as hidden in its current form. There may be niggles but I don't think there are any blockers, and there is other work (the perennial wasm32-wasi install woe) which is dependent on this and would be nice to kick off.

lann force-pushed the spin-doctor branch from 3d38d90 to abf1212 Compare April 28, 2023 19:48

lann marked this pull request as draft April 28, 2023 19:50

itowlson mentioned this pull request Apr 30, 2023

Check for Rust and wasm32-wasi on spin new and spin build #1432

Closed

lann force-pushed the spin-doctor branch 3 times, most recently from d68506c to 2e838aa Compare May 1, 2023 20:43

lann marked this pull request as ready for review May 1, 2023 20:44

michelleN added this to the 1.2.0 milestone May 1, 2023

lann force-pushed the spin-doctor branch from 2e838aa to 3e1be22 Compare May 1, 2023 21:43

calebschoepp reviewed May 1, 2023

View reviewed changes

itowlson reviewed May 2, 2023

View reviewed changes

rylev reviewed May 2, 2023

View reviewed changes

lann force-pushed the spin-doctor branch 2 times, most recently from 48e8d46 to 606ff81 Compare May 3, 2023 16:34

itowlson reviewed May 8, 2023

View reviewed changes

crates/common/src/spin_env.rs Outdated Show resolved Hide resolved

src/commands/doctor.rs Outdated Show resolved Hide resolved

src/commands/doctor.rs Outdated Show resolved Hide resolved

lann force-pushed the spin-doctor branch from 5e8a3ce to f675e1b Compare May 8, 2023 14:49

lann added 5 commits May 15, 2023 15:52

Add 'spin doctor' subcommand

e377f41

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

doctor: Fix VersionDiagnosis returning multiple diagnoses

a41a95d

While "old version key" and "wrong value" are logically different issues, from a UX perspective it seems better to let the "more critical" "wrong value" issue take precedence. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Replace Diagnose trait with Diagnostic trait

f2d661e

This changes the way diagnoses are "registered" with a Checkup; rather than storing function pointers it now stores trait objects. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Split Treatment::description into summary and dry_run

a9063f6

The former is a short description to be displayed as a choice while the latter is a longer e.g. diff of what the treatment will do. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

doctor: Fix some emoji width display issues

c28707e

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

lann force-pushed the spin-doctor branch from f675e1b to c28707e Compare May 15, 2023 19:59

doctor: Support directory or file for -f

5cbe8d6

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

itowlson approved these changes May 17, 2023

View reviewed changes

lann merged commit 2bdc2c5 into fermyon:main May 17, 2023

lann deleted the spin-doctor branch May 17, 2023 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`spin doctor` prototype #1435

`spin doctor` prototype #1435

lann commented Apr 28, 2023 •

edited

Loading

calebschoepp left a comment

itowlson left a comment

itowlson May 2, 2023

lann May 2, 2023

rylev May 2, 2023

itowlson May 2, 2023

calebschoepp May 3, 2023

rylev May 2, 2023

lann May 2, 2023

rylev May 2, 2023

itowlson left a comment


		use std::{fmt::Debug, fs, future::Future, path::PathBuf, pin::Pin, process::Command, sync::Arc};

		use anyhow::{ensure, Context, Result};

spin doctor prototype #1435

spin doctor prototype #1435

Conversation

lann commented Apr 28, 2023 • edited Loading

calebschoepp left a comment

Choose a reason for hiding this comment

itowlson left a 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

itowlson left a comment

Choose a reason for hiding this comment

`spin doctor` prototype #1435

`spin doctor` prototype #1435

lann commented Apr 28, 2023 •

edited

Loading