Skip to content
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

Validate target environments #2806

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
377 changes: 342 additions & 35 deletions Cargo.lock

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ spin-app = { path = "crates/app" }
spin-build = { path = "crates/build" }
spin-common = { path = "crates/common" }
spin-doctor = { path = "crates/doctor" }
spin-environments = { path = "crates/environments" }
spin-expressions = { path = "crates/expressions" }
spin-factor-outbound-networking = { path = "crates/factor-outbound-networking" }
spin-http = { path = "crates/http" }
Expand Down
1 change: 1 addition & 0 deletions crates/build/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ anyhow = { workspace = true }
futures = { workspace = true }
serde = { workspace = true }
spin-common = { path = "../common" }
spin-environments = { path = "../environments" }
spin-manifest = { path = "../manifest" }
subprocess = "0.2"
terminal = { path = "../terminal" }
Expand Down
80 changes: 64 additions & 16 deletions crates/build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,79 @@ use subprocess::{Exec, Redirection};
use crate::manifest::component_build_configs;

/// If present, run the build command of each component.
pub async fn build(manifest_file: &Path, component_ids: &[String]) -> Result<()> {
let (components, manifest_err) =
component_build_configs(manifest_file)
.await
.with_context(|| {
format!(
"Cannot read manifest file from {}",
quoted_path(manifest_file)
)
})?;
pub async fn build(
manifest_file: &Path,
component_ids: &[String],
skip_target_checks: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: I'm never against replacing bool with a more descriptive bool-like enum:

enum TargetChecks {
    Perform,
    Skip
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor am I @rylev! But in this case the only thing ever passed to it (except in a couple of tests) was BuildCommand::skip_target_checks (which itself emerged from clap magic) and so I didn't feel a self-descriptive enum added enough value. I agree that by default we should avoid bare booleans (and I'd want to revisit it if the function sprouted more boolean args), but in this case it didn't seem worth the candle? I am happy to be overruled.

cache_root: Option<PathBuf>,
) -> Result<()> {
let build_info = component_build_configs(manifest_file)
.await
.with_context(|| {
format!(
"Cannot read manifest file from {}",
quoted_path(manifest_file)
)
})?;
let app_dir = parent_dir(manifest_file)?;

let build_result = build_components(component_ids, components, app_dir);
let build_result = build_components(component_ids, build_info.components(), &app_dir);

if let Some(e) = manifest_err {
// Emit any required warnings now, so that they don't bury any errors.
if let Some(e) = build_info.load_error() {
// The manifest had errors. We managed to attempt a build anyway, but we want to
// let the user know about them.
terminal::warn!("The manifest has errors not related to the Wasm component build. Error details:\n{e:#}");
// Checking deployment targets requires a healthy manifest (because trigger types etc.),
// if any of these were specified, warn they are being skipped.
let should_have_checked_targets =
!skip_target_checks && build_info.has_deployment_targets();
if should_have_checked_targets {
terminal::warn!(
"The manifest error(s) prevented Spin from checking the deployment targets."
);
}
}

// If the build failed, exit with an error at this point.
build_result?;

let Some(manifest) = build_info.manifest() else {
// We can't proceed to checking (because that needs a full healthy manifest), and we've
// already emitted any necessary warning, so quit.
return Ok(());
};

if !skip_target_checks {
let application = spin_environments::ApplicationToValidate::new(
manifest.clone(),
manifest_file.parent().unwrap(),
)
.await?;
let errors = spin_environments::validate_application_against_environment_ids(
&application,
build_info.deployment_targets(),
cache_root.clone(),
&app_dir,
)
.await?;

for error in &errors {
terminal::error!("{error}");
}

if !errors.is_empty() {
anyhow::bail!("All components built successfully, but one or more was incompatible with one or more of the deployment targets.");
}
}

build_result
Ok(())
}

fn build_components(
component_ids: &[String],
components: Vec<ComponentBuildInfo>,
app_dir: PathBuf,
app_dir: &Path,
) -> Result<(), anyhow::Error> {
let components_to_build = if component_ids.is_empty() {
components
Expand Down Expand Up @@ -70,7 +118,7 @@ fn build_components(

components_to_build
.into_iter()
.map(|c| build_component(c, &app_dir))
.map(|c| build_component(c, app_dir))
.collect::<Result<Vec<_>, _>>()?;

terminal::step!("Finished", "building all Spin components");
Expand Down Expand Up @@ -148,6 +196,6 @@ mod tests {
#[tokio::test]
async fn can_load_even_if_trigger_invalid() {
let bad_trigger_file = test_data_root().join("bad_trigger.toml");
build(&bad_trigger_file, &[]).await.unwrap();
build(&bad_trigger_file, &[], true, None).await.unwrap();
}
}
124 changes: 111 additions & 13 deletions crates/build/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,119 @@ use std::{collections::BTreeMap, path::Path};

use spin_manifest::{schema::v2, ManifestVersion};

pub enum ManifestBuildInfo {
Loadable {
components: Vec<ComponentBuildInfo>,
deployment_targets: Vec<spin_manifest::schema::v2::TargetEnvironmentRef>,
manifest: spin_manifest::schema::v2::AppManifest,
},
Unloadable {
components: Vec<ComponentBuildInfo>,
has_deployment_targets: bool,
load_error: spin_manifest::Error,
},
}

impl ManifestBuildInfo {
pub fn components(&self) -> Vec<ComponentBuildInfo> {
match self {
Self::Loadable { components, .. } => components.clone(),
Self::Unloadable { components, .. } => components.clone(),
}
}

pub fn load_error(&self) -> Option<&spin_manifest::Error> {
match self {
Self::Loadable { .. } => None,
Self::Unloadable { load_error, .. } => Some(load_error),
}
}

pub fn deployment_targets(&self) -> &[spin_manifest::schema::v2::TargetEnvironmentRef] {
match self {
Self::Loadable {
deployment_targets, ..
} => deployment_targets,
Self::Unloadable { .. } => &[],
}
}

pub fn has_deployment_targets(&self) -> bool {
match self {
Self::Loadable {
deployment_targets, ..
} => !deployment_targets.is_empty(),
Self::Unloadable {
has_deployment_targets,
..
} => *has_deployment_targets,
}
}

pub fn manifest(&self) -> Option<&spin_manifest::schema::v2::AppManifest> {
match self {
Self::Loadable { manifest, .. } => Some(manifest),
Self::Unloadable { .. } => None,
}
}
}

/// Returns a map of component IDs to [`v2::ComponentBuildConfig`]s for the
/// given (v1 or v2) manifest path. If the manifest cannot be loaded, the
/// function attempts fallback: if fallback succeeds, result is Ok but the load error
/// is also returned via the second part of the return value tuple.
pub async fn component_build_configs(
manifest_file: impl AsRef<Path>,
) -> Result<(Vec<ComponentBuildInfo>, Option<spin_manifest::Error>)> {
pub async fn component_build_configs(manifest_file: impl AsRef<Path>) -> Result<ManifestBuildInfo> {
let manifest = spin_manifest::manifest_from_file(&manifest_file);
match manifest {
Ok(manifest) => Ok((build_configs_from_manifest(manifest), None)),
Err(e) => fallback_load_build_configs(&manifest_file)
.await
.map(|bc| (bc, Some(e))),
Ok(mut manifest) => {
spin_manifest::normalize::normalize_manifest(&mut manifest);
let components = build_configs_from_manifest(&manifest);
let deployment_targets = deployment_targets_from_manifest(&manifest);
Ok(ManifestBuildInfo::Loadable {
components,
deployment_targets,
manifest,
})
}
Err(load_error) => {
// The manifest didn't load, but the problem might not be build-affecting.
// Try to fall back by parsing out only the bits we need. And if something
// goes wrong with the fallback, give up and return the original manifest load
// error.
let Ok(components) = fallback_load_build_configs(&manifest_file).await else {
return Err(load_error.into());
};
let Ok(has_deployment_targets) = has_deployment_targets(&manifest_file).await else {
return Err(load_error.into());
};
Ok(ManifestBuildInfo::Unloadable {
components,
has_deployment_targets,
load_error,
})
}
}
}

fn build_configs_from_manifest(
mut manifest: spin_manifest::schema::v2::AppManifest,
manifest: &spin_manifest::schema::v2::AppManifest,
) -> Vec<ComponentBuildInfo> {
spin_manifest::normalize::normalize_manifest(&mut manifest);

manifest
.components
.into_iter()
.iter()
.map(|(id, c)| ComponentBuildInfo {
id: id.to_string(),
build: c.build,
build: c.build.clone(),
})
.collect()
}

fn deployment_targets_from_manifest(
manifest: &spin_manifest::schema::v2::AppManifest,
) -> Vec<spin_manifest::schema::v2::TargetEnvironmentRef> {
manifest.application.targets.clone()
}

async fn fallback_load_build_configs(
manifest_file: impl AsRef<Path>,
) -> Result<Vec<ComponentBuildInfo>> {
Expand All @@ -57,7 +139,23 @@ async fn fallback_load_build_configs(
})
}

#[derive(Deserialize)]
async fn has_deployment_targets(manifest_file: impl AsRef<Path>) -> Result<bool> {
let manifest_text = tokio::fs::read_to_string(manifest_file).await?;
Ok(match ManifestVersion::detect(&manifest_text)? {
ManifestVersion::V1 => false,
ManifestVersion::V2 => {
let table: toml::value::Table = toml::from_str(&manifest_text)?;
table
.get("application")
.and_then(|a| a.as_table())
.and_then(|t| t.get("targets"))
.and_then(|arr| arr.as_array())
.is_some_and(|arr| !arr.is_empty())
}
})
}

#[derive(Clone, Deserialize)]
pub struct ComponentBuildInfo {
#[serde(default)]
pub id: String,
Expand Down
2 changes: 2 additions & 0 deletions crates/compose/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ async-trait = { workspace = true }
indexmap = "2"
semver = "1"
spin-app = { path = "../app" }
spin-common = { path = "../common" }
spin-componentize = { workspace = true }
spin-serde = { path = "../serde" }
thiserror = { workspace = true }
tokio = { version = "1.23", features = ["fs"] }
wac-graph = "0.6"

[lints]
Expand Down
Loading
Loading