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

Try to load manifest during spin build #2527

Merged
merged 1 commit into from
May 23, 2024

Conversation

itowlson
Copy link
Contributor

This partly, but only partly, addresses #2520.

With this change, spin build attempts to load the full manifest, falling back to the subset only if that fails, and emitting a warning for the non-build-related validation failure. This will catch schema errors such as allowed_outbound_hsots = ...:

$ spin build
Building component self-req-test with `cargo build --target wasm32-wasi --release`
    Finished `release` profile [optimized] target(s) in 0.03s
Finished building all Spin components
Warning: The manifest has errors not related to the Wasm component build. Error details:
TOML parse error at line 18, column 1
   |
18 | allowed_outbound_hsots = []
   | ^^^^^^^^^^^^^^^^^^^^^^
unknown field `allowed_outbound_hsots`, expected one of `source`, `description`, `variables`, `environment`, `files`, `exclude_files`, `allowed_http_hosts`, `allowed_outbound_hosts`, `key_value_stores`, `sqlite_databases`, `ai_models`, `build`, `tool`

HOWEVER.

The problem in #2520 relates to variable name validation, and that does not happen at manifest load, or even lockfile creation, but in the trigger when it tries to resolve the variables. Checking that in the loader would require additional validation logic in the loader, similar to how we validate kebab-ids. Nothing stops us adding that validation logic, but it's not there now, and for this PR I didn't want to extend load-time validation (with who knows what consequences) but only run the existing load-time validation at build time. If we decide to add variable name validation at load time then spin build with this PR will inherit it.

@itowlson itowlson requested review from lann and mikkelhegn May 23, 2024 03:03
Copy link
Member

@mikkelhegn mikkelhegn left a comment

Choose a reason for hiding this comment

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

The approach in this PR looks good to me. (Not a code review :-))

@@ -8,6 +8,31 @@ use spin_manifest::{schema::v2, ManifestVersion};
/// given (v1 or v2) manifest path.
pub async fn component_build_configs(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps rename this component_build_configs_with_error or something? And update the docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not mad keen on that name (and can't think of a fully expressive one that isn't awkward) but have updated the doc string.

@itowlson itowlson enabled auto-merge May 23, 2024 20:10
@itowlson itowlson merged commit 8e99755 into fermyon:main May 23, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants