diff --git a/docs/docs/commands/lint.md b/docs/docs/commands/lint.md index 245daff..0265af0 100644 --- a/docs/docs/commands/lint.md +++ b/docs/docs/commands/lint.md @@ -11,3 +11,8 @@ When run `scope lint` will check: To validate `ScopeReportLocation`'s, inputs are generated and templates are rendered. This allows report templates to be validated before they exposed to others. +`ScopeKnownError` and `ScopeDoctorGroup` aren't checked here because they're already validated +at config load time, before any command (including `lint`) runs. An invalid `ScopeKnownError` +will fail the command outright rather than showing up as a lint finding; see +[ScopeKnownError](../models/ScopeKnownError.mdx#validation). + diff --git a/docs/docs/models/ScopeDoctorGroup.mdx b/docs/docs/models/ScopeDoctorGroup.mdx index e4ee3ac..fbf62a3 100644 --- a/docs/docs/models/ScopeDoctorGroup.mdx +++ b/docs/docs/models/ScopeDoctorGroup.mdx @@ -89,6 +89,12 @@ summary; its dependencies that get trimmed along with it are not. A command can either be relative, or use the PATH. To target a script relative to the group it must start with `.`, and giving a relative path to the group file. +## Validation + +A `ScopeDoctorGroup` that fails to load (for example, a `check` or `fix` command whose template +fails to render) is logged as a warning naming the file, and skipped. The rest of your +configuration still loads normally. + ## Schema diff --git a/docs/docs/models/ScopeKnownError.mdx b/docs/docs/models/ScopeKnownError.mdx index 3c8d139..9487f0d 100644 --- a/docs/docs/models/ScopeKnownError.mdx +++ b/docs/docs/models/ScopeKnownError.mdx @@ -21,6 +21,13 @@ spec: help: The command had an error, try reading the logs around there to find out what happened. ``` +## Validation + +An invalid `ScopeKnownError` (a pattern with a reserved capture group name like `captures` or +`working_dir`, or a `fix` whose template fails to render) causes `scope` to fail to load +entirely, rather than silently ignoring that one check. A known error that isn't actually +registered would otherwise give false confidence that `scope` is watching for it. + ## Schema diff --git a/docs/docs/models/ScopeReportLocation.mdx b/docs/docs/models/ScopeReportLocation.mdx index 5dafcef..7e58cd1 100644 --- a/docs/docs/models/ScopeReportLocation.mdx +++ b/docs/docs/models/ScopeReportLocation.mdx @@ -65,6 +65,11 @@ spec: url: http://localhost:8000 ``` +### Validation + +A `ScopeReportLocation` that fails to load is logged as a warning naming the file, and skipped. +The rest of your configuration still loads normally. + ### Schema diff --git a/src/shared/config_load.rs b/src/shared/config_load.rs index 4274dbd..040e94e 100644 --- a/src/shared/config_load.rs +++ b/src/shared/config_load.rs @@ -70,7 +70,7 @@ impl ConfigOptions { }; let config_path = self.find_scope_paths(&working_dir); - let found_config = FoundConfig::new(self, working_dir, config_path).await; + let found_config = FoundConfig::new(self, working_dir, config_path).await?; debug!("Loaded config {:?}", found_config); @@ -132,7 +132,7 @@ impl FoundConfig { config_options: &ConfigOptions, working_dir: PathBuf, config_path: Vec, - ) -> Self { + ) -> Result { let default_path = std::env::var("PATH").unwrap_or_default(); let mut config_path = config_path.to_vec(); @@ -164,13 +164,29 @@ impl FoundConfig { run_id: config_options.get_run_id(), }; + let mut known_error_failures: Vec = Vec::new(); for raw_config in raw_config { - if let Ok(value) = raw_config.try_into() { - this.add_model(value); + let file_path = raw_config.metadata().file_path(); + let kind = raw_config.kind.clone(); + match ParsedConfig::try_from(raw_config) { + Ok(value) => this.add_model(value), + Err(e) => { + let message = + format!("Unable to convert config from {} because {}", file_path, e); + if is_hard_fail_kind(&kind) { + known_error_failures.push(message); + } else { + warn!(target: "user", "{}", message); + } + } } } - this + if !known_error_failures.is_empty() { + return Err(anyhow!(known_error_failures.join("\n"))); + } + + Ok(this) } pub fn write_raw_config_to_disk(&self) -> Result { @@ -205,6 +221,13 @@ impl FoundConfig { } } +/// A `ScopeKnownError` that fails to load is worse than useless: it looks like the tool is +/// watching for that error when it isn't. Other kinds degrade gracefully (warn and continue), +/// but known-error failures abort config load entirely. +fn is_hard_fail_kind(kind: &str) -> bool { + kind.eq_ignore_ascii_case("ScopeKnownError") +} + fn insert_if_absent(map: &mut BTreeMap, entry: T) { let name = entry.name().to_string(); if map.contains_key(&name) { diff --git a/tests/scope_root.rs b/tests/scope_root.rs index e1edc8d..10cf2a4 100644 --- a/tests/scope_root.rs +++ b/tests/scope_root.rs @@ -70,3 +70,145 @@ spec: )); test_helper.clean_work_dir(); } + +#[test] +fn test_known_error_with_reserved_capture_name_fails_load() { + let test_helper = ScopeTestHelper::new( + "test_known_error_with_reserved_capture_name_fails_load", + "empty", + ); + let example_file = "apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-reserved + description: uses a reserved capture group name +spec: + pattern: \"boom: (?.*)\" + help: \"should never load\" +"; + test_helper + .work_dir + .child(".scope/bad-reserved.yaml") + .write_str(example_file) + .unwrap(); + + let result = test_helper.run_command(&["list"]); + + result + .failure() + .stdout(predicate::str::contains("bad-reserved.yaml")) + .stdout(predicate::str::contains("reserved")); + test_helper.clean_work_dir(); +} + +#[test] +fn test_known_error_with_broken_fix_template_fails_load() { + let test_helper = ScopeTestHelper::new( + "test_known_error_with_broken_fix_template_fails_load", + "empty", + ); + let example_file = "apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-fix-template + description: has a broken fix template +spec: + pattern: \"boom: (.*)\" + help: \"should never load\" + fix: + commands: + - \"echo {{ unclosed\" +"; + test_helper + .work_dir + .child(".scope/bad-fix-template.yaml") + .write_str(example_file) + .unwrap(); + + let result = test_helper.run_command(&["list"]); + + result + .failure() + .stdout(predicate::str::contains("bad-fix-template.yaml")); + test_helper.clean_work_dir(); +} + +#[test] +fn test_multiple_known_error_failures_are_all_reported() { + let test_helper = ScopeTestHelper::new( + "test_multiple_known_error_failures_are_all_reported", + "empty", + ); + let bad_reserved = "apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-reserved + description: uses a reserved capture group name +spec: + pattern: \"boom: (?.*)\" + help: \"should never load\" +"; + let bad_fix_template = "apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-fix-template + description: has a broken fix template +spec: + pattern: \"boom: (.*)\" + help: \"should never load\" + fix: + commands: + - \"echo {{ unclosed\" +"; + test_helper + .work_dir + .child(".scope/bad-reserved.yaml") + .write_str(bad_reserved) + .unwrap(); + test_helper + .work_dir + .child(".scope/bad-fix-template.yaml") + .write_str(bad_fix_template) + .unwrap(); + + let result = test_helper.run_command(&["list"]); + + result + .failure() + .stdout(predicate::str::contains("bad-reserved.yaml")) + .stdout(predicate::str::contains("bad-fix-template.yaml")); + test_helper.clean_work_dir(); +} + +#[test] +fn test_doctor_group_broken_template_warns_and_continues() { + let test_helper = ScopeTestHelper::new( + "test_doctor_group_broken_template_warns_and_continues", + "empty", + ); + let example_file = "apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: broken-template-group + description: has a broken check command template +spec: + actions: + - name: broken-check + description: check with a broken template + check: + commands: + - \"echo {{ unclosed\" +"; + test_helper + .work_dir + .child(".scope/broken-template-group.yaml") + .write_str(example_file) + .unwrap(); + + let result = test_helper.run_command(&["list"]); + + result + .success() + .stdout(predicate::str::contains("broken-template-group.yaml")); + test_helper.clean_work_dir(); +}