Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions crates/openjd-model/src/template/validate_v2023_09/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,12 @@ pub struct EffectiveRules {
pub allowed_job_param_types: std::collections::HashSet<JobParameterType>,
pub allowed_task_param_types: std::collections::HashSet<TaskParameterType>,
pub allow_fmtstring_in_numeric_fields: bool,
/// When the `WRAP_ACTIONS` extension is enabled, an environment script
/// is valid if it defines any of `onEnter`, `onExit`, or one of the
/// RFC 0008 wrap hooks (`onWrapEnvEnter`, `onWrapTaskRun`, `onWrapEnvExit`).
/// Without the extension, the base 2023-09 rule applies:
/// `onEnter` is required whenever `script` is present.
/// An environment script must define at least one action whether or not
/// this flag is set, and either `onEnter` or `onExit` alone satisfies
/// that. The flag decides two things: whether the RFC 0008 wrap hooks
/// (`onWrapEnvEnter`, `onWrapTaskRun`, `onWrapEnvExit`) also count as an
/// acceptable choice, and which wording the error uses when no action is
/// defined. It does not make `onEnter` required.
pub wrap_actions_enabled: bool,
}

Expand Down
39 changes: 16 additions & 23 deletions crates/openjd-model/src/template/validate_v2023_09/structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,29 +389,22 @@ pub fn validate_single_environment(
if let Some(script) = &env.script {
let script_path = path_field(path, "script");
let actions_path = path_field(&script_path, "actions");
// Base 2023-09 requires `onEnter` whenever a `script` is present.
// RFC 0008 relaxes this: when `WRAP_ACTIONS` is enabled, an env may
// define only wrap hooks (or any single action) without a standalone
// `onEnter`. Concretely, require at least one of the five known
// actions to be present so we don't accept an empty `actions: {}`.
if rules.wrap_actions_enabled {
// RFC 0008: an env may define only wrap hooks (or any single
// action) without a standalone `onEnter`. Require at least one of
// the five known actions so an empty `actions: {}` is still
// rejected. The all-or-nothing wrap-hook rule (defining any wrap
// hook requires all three) is enforced separately in
// `wrap_actions.rs`, not here.
if !script.actions.has_any_action() {
errors.add(
&actions_path,
"must define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together).",
);
}
} else if script.actions.on_enter.is_none() {
// Preserve the original wording when the extension is not enabled
// so pre-RFC error messages don't change. `on_enter.is_none()`
// also covers the empty-`actions` case.
errors.add(&actions_path, "onEnter is required.");
// An environment script must define at least one action. Either
// `onEnter` or `onExit` alone satisfies this, so an environment whose
// only work is cleanup is valid. With `WRAP_ACTIONS` a complete set of
// wrap hooks also satisfies it. Requiring at least one of the five
// known actions is what rejects an empty `actions: {}`. The
// all-or-nothing wrap-hook rule (defining any wrap hook requires all
// three) is enforced separately in `wrap_actions.rs`, not here.
if !script.actions.has_any_action() {
Comment thread
leongdl marked this conversation as resolved.
errors.add(
&actions_path,
if rules.wrap_actions_enabled {
"must define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together)."
} else {
"must define at least one of onEnter or onExit."
},
);
}
Comment thread
mwiebe marked this conversation as resolved.
for (name, action) in script.actions.iter_named() {
validate_action(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ fn test_env_actions_empty() {
}"#;
check_err(
s,
&["jobEnvironments[0] -> script -> actions:\n\tonEnter is required."],
&["jobEnvironments[0] -> script -> actions:\n\tmust define at least one of onEnter or onExit."],
);
}

Expand Down Expand Up @@ -599,10 +599,9 @@ fn test_env_action_on_exit() {
"steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "foo"}}}}],
"jobEnvironments": [{"name": "E", "script": {"actions": {"onExit": {"command": "foo"}}}}]
}"#;
check_err(
s,
&["jobEnvironments[0] -> script -> actions:\n\tonEnter is required."],
);
// Either ordinary action alone is sufficient, so a cleanup-only
// environment is valid.
decode_ok(s);
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -744,18 +744,41 @@ fn test_env_name_513_chars_with_extension_fails() {
}

// ══════════════════════════════════════════════════════════════
// Bug fix: onEnter is required per spec §4.3
// Environment actions: at least one of onEnter or onExit (spec §4.3)
// ══════════════════════════════════════════════════════════════

#[test]
fn test_env_actions_on_enter_required() {
// onExit alone should fail — onEnter is required
check_env_err(
fn test_env_actions_on_exit_only_is_valid() {
// onExit alone is sufficient: a queue environment whose only work is
// cleanup does not need an onEnter.
decode_ok(
r#"{
"specificationVersion": "environment-2023-09",
"environment": {"name": "Foo", "script": {"actions": {"onExit": {"command": "cleanup"}}}}
}"#,
&["environment -> script -> actions:\n\tonEnter is required."],
);
}

#[test]
fn test_env_actions_on_enter_only_is_valid() {
decode_ok(
r#"{
"specificationVersion": "environment-2023-09",
"environment": {"name": "Foo", "script": {"actions": {"onEnter": {"command": "setup"}}}}
}"#,
);
}

#[test]
fn test_env_actions_empty_is_rejected() {
// Removing the onEnter requirement must not make an empty actions object
// acceptable.
check_env_err(
r#"{
"specificationVersion": "environment-2023-09",
"environment": {"name": "Foo", "script": {"actions": {}}}
}"#,
&["environment -> script -> actions:\n\tmust define at least one of onEnter or onExit."],
Comment thread
leongdl marked this conversation as resolved.
);
}

Expand Down
25 changes: 25 additions & 0 deletions crates/openjd-model/tests/integration/test_wrap_actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,31 @@ fn defining_zero_wrap_hooks_accepted() {
);
}

// ════════════════════════════════════════════════════════════════════
// At-least-one-action rule, WRAP_ACTIONS wording
//
// An environment script must define at least one action. With
// WRAP_ACTIONS enabled the error names the wrap hooks as a third
// acceptable choice alongside onEnter and onExit. The wording without
// the extension is covered in test_environment_template.rs.
// ════════════════════════════════════════════════════════════════════

#[test]
fn empty_env_actions_rejected_with_wrap_hooks_named() {
expect_env_err(
r#"{
"specificationVersion": "environment-2023-09",
"extensions": ["WRAP_ACTIONS", "EXPR"],
"environment": {
"name": "Empty",
"script": {"actions": {}}
}
}"#,
WRAP_EXTS,
&["environment -> script -> actions:\n\tmust define at least one of onEnter or onExit, or the complete set of wrap hooks (onWrapEnvEnter, onWrapTaskRun, and onWrapEnvExit together)."],
);
}

// ════════════════════════════════════════════════════════════════════
// EXPR prerequisite (RFC 0008)
//
Expand Down
Loading