Add spin targets check command - #3635
Conversation
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
itowlson
left a comment
itowlson
left a comment
There was a problem hiding this comment.
A few consistency quirks, but the main issue is how to handle per-component targets already declared in the app.
I am pretty sure the proposed behaviour is incorrect if the user runs spin target check to check against the env in the manifest. I feel like this should succeed iff spin build succeeds - it would be confusing for spin build to say "you're good to go" and "spin targets check" to say "not so fast, buddy." I am not sure what the correct behaviour is if the user runs spin target check -E - on the one hand, the manifest says "hey these components are gonna run in a different environment", on the other hand, you did ask if the app would run in this specific environment.
Understanding the use cases might be helpful for figuring out the right answer. I find the ad hoc validation use case a bit hard to get my head around so I am probably not seeing the context that would help clarify this!
| /// example `spin-up@3.6`), an `http:` URL, or a `file:` path, and may be specified | ||
| /// multiple times. If omitted, the targets declared in the application manifest | ||
| /// are checked. | ||
| #[clap(short = 'E', long = "target")] |
There was a problem hiding this comment.
Elsewhere the long form is defaulted to --target-environment.
|
|
||
| /// Check the application as already built, rather than building it first. | ||
| #[clap(long)] | ||
| pub no_build: bool, |
There was a problem hiding this comment.
In most other commands, the default is to not build; --build is the option that can be added. The counterexample is spin watch, but that's essentially build and run commands in a trenchcoat, and I'm not sure this is. But agree this one can be argued either way.
|
|
||
| /// Check the application as already built, rather than building it first. | ||
| #[clap(long)] | ||
| pub no_build: bool, |
There was a problem hiding this comment.
I believe our usual convention for things like this is skip- (cf. spin watch --skip-build, spin build --skip-target-checks).
There was a problem hiding this comment.
Changing this to build.
| pub no_build: bool, | ||
| } | ||
|
|
||
| async fn check(cmd: CheckCommand) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Everywhere else the convention is impl FooCmd { async fn run(&self) } - is there a reason for doing this differently?
(Okay, there are exceptions like spin plugins update, but I think those are only when the command has no arguments.)
| profile, | ||
| &[], | ||
| spin_build::TargetChecking::Skip, | ||
| spin_build::GenerateDependencyWits::Generate, |
There was a problem hiding this comment.
It feels a bit weird to me that running a check command would modify the dependency WITs - a check command feels like it should be non-side-effectful. I guess it's possble that build would fail if we didn't, is that the idea?
| // Environments named on the command line take precedence; otherwise fall back | ||
| // to the targets declared in the manifest. | ||
| let target_refs: Vec<TargetEnvironmentRef> = if cmd.target_environment.is_empty() { | ||
| manifest.application.targets.clone() |
There was a problem hiding this comment.
This seems equivalent to a spin build. Is the idea to enable spin targets check --skip-build to validate without rebuilding?
| .collect() | ||
| }; | ||
|
|
||
| if target_refs.is_empty() { |
There was a problem hiding this comment.
Maybe we should check this before slogging through a potentially lengthy build!
|
|
||
| let targets = spin_environments::Targets { | ||
| default: &target_refs, | ||
| overrides: Default::default(), |
There was a problem hiding this comment.
Hmm, this is interesting. If the app says "but hey I intend to deploy the draw-an-owl component in the mighty-gpu environment," is it correct to fail a spin targets check with "no, draw-an-owl doesn't work in the app environment"? (Almost certainly not.) Is it correct to fail a spin targets check -E no-gpus-here on draw-an-owl? (I am not sure.)
|
|
||
| if validation.is_ok() { | ||
| terminal::step!( | ||
| "Compatible", |
There was a problem hiding this comment.
If I read the step macro right, this will print "Compatible the application is compatible..." Is that what you see or am I misreading? If that's correct, we should add some punctuation.
|
Also realised, if we are doing this then I think we should add |
|
It occurred to me this should force a catalogue update if the env is unversioned - we don't do this during build because we don't want to delay every. single. build. but when the user specifically asks for a check I think it's worth the network hop to make sure they're checking the latest env. |
2 participants
No description provided.