Allow an environment to define only onExit - #346
Conversation
Review feedback statusBoth code findings are fixed in c862472. I have resolved those two threads and left the third open, because it is a dependency rather than a defect.
Fixed: stale doc commentThe doc comment on Fixed: missing test for the WRAP_ACTIONS wordingAdded I confirmed the gap was real instead of assuming it. Swapping the two arms of the Verification after the fix:
Open: merge orderThe three Conformance failures are one case, openjd-specifications#178 renames the fixture to drop the This PR should merge after openjd-specifications#178. Merging first would turn Related: openjd-model-for-python#338 makes the same change in the Python model. |
An environment script that defined onExit without onEnter was rejected during template validation with "onEnter is required." This blocked environments whose only work is cleanup, including queue environments that tear down state established elsewhere in the session. Apply the same "at least one action" rule regardless of whether WRAP_ACTIONS is enabled, so either ordinary action alone is sufficient. The error message keeps naming the wrap hooks only when the extension is enabled, since they are not a valid choice otherwise. An actions object that defines no action is still rejected, and the all-or-nothing wrap-hook rule in wrap_actions.rs is unchanged. The session runtime already handles an absent onEnter. enter_environment guards on the action being present, and the environment script runner treats a missing action as a successful no-op, which is the path a variables-only environment already takes. This matches openjd-model-for-python PR 338 and the wiki correction in openjd-specifications PR 178. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
… wording Address review feedback on the at-least-one-action change. The doc comment on EffectiveRules::wrap_actions_enabled still said onEnter is required when the extension is absent, which the previous commit made untrue. Describe what the flag now decides: whether the wrap hooks count as an acceptable choice, and which wording the error uses. A future reader could otherwise reintroduce the removed branch to match the comment. Add the missing test for the WRAP_ACTIONS wording. Only the plain arm was asserted, so swapping the two message arms went undetected. The new test asserts the full error text for an empty actions object with the extension enabled, and that swap now fails three tests. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
c862472 to
585184c
Compare
What was the problem/requirement? (What/Why)
An environment script that defines
onExitwithoutonEnteris rejected during template validation:This blocks environments whose only work is cleanup. The motivating case is a queue environment that tears down state established elsewhere in the session. The same
<Environment>type backs standalone environment templates,jobEnvironments, and step environments, so all three were affected.The rule came from reading the 2023-09 wiki literally: line 1572 declares
onEnter: <Action>with no@optionalannotation. That annotation was never present and was never removed, so the requirement has been in the document since the specification was first imported in November 2023. openjd-specifications PR 178 corrects the wiki, annotating bothonEnterandonExitas optional and stating the at-least-one constraint explicitly.What was the solution? (How)
Apply the existing "at least one action" rule regardless of whether
WRAP_ACTIONSis enabled, incrates/openjd-model/src/template/validate_v2023_09/structure.rs. Either ordinary action alone now satisfies validation.The error message still names the wrap hooks only when the extension is enabled, because they are not a valid choice otherwise:
WRAP_ACTIONS:must define at least one of onEnter or onExit.WRAP_ACTIONS: unchanged, naming the complete hook set as a third option.Unchanged: an
actionsobject defining no action is still rejected, and the all-or-nothing wrap-hook rule inwrap_actions.rsis untouched.Runtime safety. No session change was needed.
on_enteris alreadyOption<Action>.Session::enter_environmentguards on the action being present before running it, so theexpectinside that branch stays unreachable.EnvScriptRunner::run_env_actionreturnsSuccessfor aNoneaction, which is the path avariables:-only environment already takes. AnonExit-only environment therefore enters as a no-op and runs its exit action normally.What is the impact of this change?
One verdict changes, and it widens what is accepted. Nothing that validated before now fails.
actionsonEnteronlyonEnterandonExitonExitonlyThe error text for an empty
actionsobject withoutWRAP_ACTIONSchanges fromonEnter is required.tomust define at least one of onEnter or onExit.Callers matching on that string need updating.This change aligns openjd-rs with openjd-model-for-python PR 338. Without it the two implementations disagree on
onExit-only input.How was this change tested?
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscleancargo test --workspace: 7263 passed, 0 failed, 24 ignoredbase/job_templates/3.5--env-script-onexit-only.invalid.yaml, which asserts the old rejection and is reclassified as valid by openjd-specifications PR 178. Before this change the same suite reported 1162 passed against that fixture tree.Tests changed:
test_environment_template.rs: replaced the previousonEnter-required test withonExit-only accepted, addedonEnter-only accepted, and added emptyactionsrejected asserting the full messagetest_actions_and_steps.rs:test_env_action_on_exitnow expects success, andtest_env_actions_emptyasserts the new messageBoth new behaviors were mutation-checked. Restoring the strict requirement fails the two
onExit-only tests. Disabling the at-least-one check fails the two empty-actions tests. The mutants were removed and the source restored before committing.Was this change documented?
specs/model/public-api.mddocumentson_enter: Option<Action>, which is unchanged. No spec document states the validation rule, so no update was required. The code comment explaining the rule was rewritten.Is this a breaking change?
Not for templates, since the change only widens acceptance. It does change one validation error string, which is a behavior change for any caller matching on the old text.
Does this change impact security?
No. No files, directories, permissions, or process boundaries are affected. The runtime executes the same actions it did before, and an absent
onEnterruns nothing.Related