fix(cli): warn on --trigger-kind without --trigger-config (#257)
InjectAnalysisKind returns null on a null/empty trigger-config, so passing --trigger-kind strict WITHOUT --trigger-config silently dropped the kind on template script add/update and template alarm update. The CLI now detects that combination (TriggerKindWillBeIgnored) and prints a clear warning to stderr, then continues (warn-and-continue: the kind is advisory metadata, not a required field, so the entity is still created — just without the requested analysis kind). The --trigger-kind help text on all three commands now documents that it requires --trigger-config, as does the CLI README. This commit also carries the shared CLI command-builder file (Commands/TemplateCommands.cs) and README, which the same builders edit for both #257 and the #54 flag additions — the #54 message contracts/handler/UI/tests landed in the preceding commit. - TriggerKindWillBeIgnored predicate + WarnIfTriggerKindIgnored stderr warning, wired into script add/update and alarm update SetActions. - Shared option descriptions document the --trigger-config requirement. - Adds the #54 CLI flags (--min-time-between-runs, --execution-timeout-seconds) and TryParseMinTimeBetweenRuns to the same builder file. - Tests: TemplateTriggerKindIgnoredTests pins the warn predicate.
This commit is contained in:
Joseph Doherty
@@ -241,6 +241,28 @@ public static class TemplateCommands
|
||||
internal const string ElementTypeOptionDescription =
|
||||
"Element scalar type for a List attribute (String, Int32, Float, Double, Boolean, DateTime). Required when --data-type is List.";
|
||||
|
||||
/// <summary>Shared description for the <c>--min-time-between-runs</c> option on script add/update.</summary>
|
||||
internal const string MinTimeBetweenRunsOptionDescription =
|
||||
"Minimum time between script runs (a throttle for triggered scripts; the re-fire interval for a WhileTrue trigger). "
|
||||
+ "Accepts a value with a unit suffix: ms, s/sec, or m/min (e.g. 500ms, 5s, 2min); a bare number is seconds. "
|
||||
+ "Omit, or pass 0 / a blank value, to leave it unset (no throttle).";
|
||||
|
||||
/// <summary>Shared description for the <c>--execution-timeout-seconds</c> option on script add/update.</summary>
|
||||
internal const string ExecutionTimeoutOptionDescription =
|
||||
"Per-script execution timeout in seconds. Omit (or pass a non-positive value) to use the site's global "
|
||||
+ "default (SiteRuntimeOptions.ScriptExecutionTimeoutSeconds).";
|
||||
|
||||
/// <summary>Shared description for the script <c>--trigger-kind</c> option on script add/update.</summary>
|
||||
internal const string ScriptTriggerKindOptionDescription =
|
||||
"Expression trigger analysis kind: advisory (default) or strict. "
|
||||
+ "Strict escalates a blank expression from a warning to a deploy-blocking error. "
|
||||
+ "Requires --trigger-config: with no config the kind is ignored (a warning is printed to stderr).";
|
||||
|
||||
/// <summary>Shared description for the alarm <c>--trigger-kind</c> option on alarm update.</summary>
|
||||
internal const string AlarmTriggerKindOptionDescription =
|
||||
"Expression trigger analysis kind: advisory (default) or strict. "
|
||||
+ "Requires --trigger-config: with no config the kind is ignored (a warning is printed to stderr).";
|
||||
|
||||
/// <summary>
|
||||
/// The element scalar types permitted for a List attribute — derived from the
|
||||
/// single source of truth, <see cref="AttributeValueCodec.IsValidElementType"/>,
|
||||
@@ -321,6 +343,94 @@ public static class TemplateCommands
|
||||
private static string? NormalizeElementType(string? elementType)
|
||||
=> string.IsNullOrWhiteSpace(elementType) ? null : elementType.Trim();
|
||||
|
||||
/// <summary>
|
||||
/// Parses the raw <c>--min-time-between-runs</c> value into a <see cref="TimeSpan"/>.
|
||||
/// A blank/absent value means "unset" (<paramref name="value"/> null → <c>null</c>, no error).
|
||||
/// Otherwise the value must be a positive integer with an optional unit suffix:
|
||||
/// <c>ms</c>, <c>s</c>/<c>sec</c>, or <c>m</c>/<c>min</c>; a bare number is interpreted as
|
||||
/// seconds (matching the UI duration input's default unit). A value of <c>0</c> resolves to
|
||||
/// <c>null</c> (unset), mirroring <c>DurationInput.Compose</c>. The unit set and "bare = seconds"
|
||||
/// default deliberately mirror the Central UI duration input so the two authoring surfaces agree.
|
||||
/// </summary>
|
||||
/// <param name="value">The raw flag value, or null when the flag was omitted.</param>
|
||||
/// <param name="duration">The parsed duration, or <c>null</c> for an absent/unset value.</param>
|
||||
/// <param name="error">A descriptive error message when parsing fails; otherwise null.</param>
|
||||
/// <returns><c>true</c> when the value is absent or parses cleanly; <c>false</c> on a malformed value.</returns>
|
||||
internal static bool TryParseMinTimeBetweenRuns(string? value, out TimeSpan? duration, out string? error)
|
||||
{
|
||||
duration = null;
|
||||
error = null;
|
||||
|
||||
var trimmed = value?.Trim();
|
||||
if (string.IsNullOrEmpty(trimmed))
|
||||
return true; // omitted → unset
|
||||
|
||||
// Split a trailing alphabetic unit suffix from the leading numeric part.
|
||||
var splitIndex = trimmed.Length;
|
||||
while (splitIndex > 0 && char.IsLetter(trimmed[splitIndex - 1]))
|
||||
splitIndex--;
|
||||
|
||||
var numberPart = trimmed[..splitIndex];
|
||||
var unitPart = trimmed[splitIndex..].ToLowerInvariant();
|
||||
|
||||
if (!long.TryParse(numberPart, System.Globalization.NumberStyles.Integer, System.Globalization.CultureInfo.InvariantCulture, out var amount)
|
||||
|| amount < 0)
|
||||
{
|
||||
error = $"Invalid --min-time-between-runs '{value}'. Expected a non-negative number with an optional "
|
||||
+ "unit suffix (ms, s/sec, m/min); a bare number is seconds (e.g. 500ms, 5s, 2min).";
|
||||
return false;
|
||||
}
|
||||
|
||||
var factorMs = unitPart switch
|
||||
{
|
||||
"ms" => 1L,
|
||||
"" or "s" or "sec" => 1000L,
|
||||
"m" or "min" => 60000L,
|
||||
_ => -1L,
|
||||
};
|
||||
|
||||
if (factorMs < 0)
|
||||
{
|
||||
error = $"Invalid --min-time-between-runs unit in '{value}'. Valid units are: ms, s/sec, m/min.";
|
||||
return false;
|
||||
}
|
||||
|
||||
// 0 (with or without a unit) → unset, mirroring DurationInput.Compose's non-positive handling.
|
||||
duration = amount == 0 ? null : TimeSpan.FromMilliseconds(amount * factorMs);
|
||||
return true;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Determines whether <c>--trigger-kind</c> was supplied without a <c>--trigger-config</c>.
|
||||
/// In that case <see cref="TriggerConfigJson.InjectAnalysisKind"/> has no JSON object to inject
|
||||
/// into and silently drops the kind (#257) — the caller should warn the user.
|
||||
/// </summary>
|
||||
/// <param name="triggerConfig">The raw <c>--trigger-config</c> JSON value, or null when absent.</param>
|
||||
/// <param name="triggerKind">The raw <c>--trigger-kind</c> value, or null when absent.</param>
|
||||
/// <returns><c>true</c> when a kind was given but no config — i.e. the kind will be ignored.</returns>
|
||||
internal static bool TriggerKindWillBeIgnored(string? triggerConfig, string? triggerKind)
|
||||
=> !string.IsNullOrWhiteSpace(triggerKind) && string.IsNullOrWhiteSpace(triggerConfig);
|
||||
|
||||
/// <summary>
|
||||
/// Warns (to stderr) when <c>--trigger-kind</c> is supplied without a <c>--trigger-config</c>
|
||||
/// (see <see cref="TriggerKindWillBeIgnored"/>). The command still proceeds — the kind is
|
||||
/// advisory metadata, not a required field — but the user is told it had no effect so they are
|
||||
/// not surprised that a <c>strict</c> request did not take. Warn-and-continue (rather than a hard
|
||||
/// error) keeps the flag combination non-fatal: a script/alarm is still created, just without the
|
||||
/// requested analysis kind.
|
||||
/// </summary>
|
||||
/// <param name="triggerConfig">The raw <c>--trigger-config</c> JSON value, or null when absent.</param>
|
||||
/// <param name="triggerKind">The raw <c>--trigger-kind</c> value, or null when absent.</param>
|
||||
private static void WarnIfTriggerKindIgnored(string? triggerConfig, string? triggerKind)
|
||||
{
|
||||
if (TriggerKindWillBeIgnored(triggerConfig, triggerKind))
|
||||
{
|
||||
Console.Error.WriteLine(
|
||||
$"warning: --trigger-kind '{triggerKind!.Trim()}' was ignored because no --trigger-config was supplied. "
|
||||
+ "The analysis kind is written into the trigger-config JSON, so it has no effect without one.");
|
||||
}
|
||||
}
|
||||
|
||||
private static Command BuildAlarm(Option<string> urlOption, Option<string> formatOption, Option<string> usernameOption, Option<string> passwordOption)
|
||||
{
|
||||
var group = new Command("alarm") { Description = "Manage template alarms" };
|
||||
@@ -417,7 +527,7 @@ public static class TemplateCommands
|
||||
// M9-T28b: --trigger-kind for update (same semantics as add)
|
||||
var updateTriggerKindOption = new Option<string?>("--trigger-kind")
|
||||
{
|
||||
Description = "Expression trigger analysis kind: advisory (default) or strict."
|
||||
Description = AlarmTriggerKindOptionDescription
|
||||
};
|
||||
|
||||
var updateCmd = new Command("update") { Description = "Update a template alarm" };
|
||||
@@ -431,8 +541,10 @@ public static class TemplateCommands
|
||||
updateCmd.Add(updateTriggerKindOption);
|
||||
updateCmd.SetAction(async (ParseResult result) =>
|
||||
{
|
||||
var rawConfig = result.GetValue(updateTriggerConfigOption);
|
||||
WarnIfTriggerKindIgnored(rawConfig, result.GetValue(updateTriggerKindOption));
|
||||
var triggerConfig = TriggerConfigJson.InjectAnalysisKind(
|
||||
result.GetValue(updateTriggerConfigOption),
|
||||
rawConfig,
|
||||
result.GetValue(updateTriggerKindOption));
|
||||
return await CommandHelpers.ExecuteCommandAsync(
|
||||
result, urlOption, formatOption, usernameOption, passwordOption,
|
||||
@@ -542,12 +654,12 @@ public static class TemplateCommands
|
||||
|
||||
var paramsOption = new Option<string?>("--parameters") { Description = "Parameter definitions JSON" };
|
||||
var returnOption = new Option<string?>("--return-def") { Description = "Return definition JSON" };
|
||||
var minTimeOption = new Option<string?>("--min-time-between-runs") { Description = MinTimeBetweenRunsOptionDescription };
|
||||
var execTimeoutOption = new Option<int?>("--execution-timeout-seconds") { Description = ExecutionTimeoutOptionDescription };
|
||||
// M9-T28b: analysis kind for Expression triggers (advisory|strict; default advisory).
|
||||
var scriptTriggerKindOption = new Option<string?>("--trigger-kind")
|
||||
{
|
||||
Description = "Expression trigger analysis kind: advisory (default) or strict. " +
|
||||
"Strict escalates a blank expression from a warning to a deploy-blocking error. " +
|
||||
"Used with --trigger-config or appended to a built config when --trigger-type is Expression."
|
||||
Description = ScriptTriggerKindOptionDescription
|
||||
};
|
||||
|
||||
var addCmd = new Command("add") { Description = "Add a script to a template" };
|
||||
@@ -559,11 +671,21 @@ public static class TemplateCommands
|
||||
addCmd.Add(lockedOption);
|
||||
addCmd.Add(paramsOption);
|
||||
addCmd.Add(returnOption);
|
||||
addCmd.Add(minTimeOption);
|
||||
addCmd.Add(execTimeoutOption);
|
||||
addCmd.Add(scriptTriggerKindOption);
|
||||
addCmd.SetAction(async (ParseResult result) =>
|
||||
{
|
||||
if (!TryParseMinTimeBetweenRuns(result.GetValue(minTimeOption), out var minTime, out var minTimeError))
|
||||
{
|
||||
OutputFormatter.WriteError(minTimeError!, "INVALID_ARGUMENT");
|
||||
return 1;
|
||||
}
|
||||
|
||||
var rawConfig = result.GetValue(triggerConfigOption);
|
||||
WarnIfTriggerKindIgnored(rawConfig, result.GetValue(scriptTriggerKindOption));
|
||||
var triggerConfig = TriggerConfigJson.InjectAnalysisKind(
|
||||
result.GetValue(triggerConfigOption),
|
||||
rawConfig,
|
||||
result.GetValue(scriptTriggerKindOption));
|
||||
return await CommandHelpers.ExecuteCommandAsync(
|
||||
result, urlOption, formatOption, usernameOption, passwordOption,
|
||||
@@ -575,7 +697,9 @@ public static class TemplateCommands
|
||||
triggerConfig,
|
||||
result.GetValue(lockedOption),
|
||||
result.GetValue(paramsOption),
|
||||
result.GetValue(returnOption)));
|
||||
result.GetValue(returnOption),
|
||||
minTime,
|
||||
result.GetValue(execTimeoutOption)));
|
||||
});
|
||||
group.Add(addCmd);
|
||||
|
||||
@@ -589,10 +713,12 @@ public static class TemplateCommands
|
||||
|
||||
var updateParamsOption = new Option<string?>("--parameters") { Description = "Parameter definitions JSON" };
|
||||
var updateReturnOption = new Option<string?>("--return-def") { Description = "Return definition JSON" };
|
||||
var updateMinTimeOption = new Option<string?>("--min-time-between-runs") { Description = MinTimeBetweenRunsOptionDescription };
|
||||
var updateExecTimeoutOption = new Option<int?>("--execution-timeout-seconds") { Description = ExecutionTimeoutOptionDescription };
|
||||
// M9-T28b: --trigger-kind for update (same semantics as add)
|
||||
var updateScriptTriggerKindOption = new Option<string?>("--trigger-kind")
|
||||
{
|
||||
Description = "Expression trigger analysis kind: advisory (default) or strict."
|
||||
Description = ScriptTriggerKindOptionDescription
|
||||
};
|
||||
|
||||
var updateCmd = new Command("update") { Description = "Update a template script" };
|
||||
@@ -604,11 +730,21 @@ public static class TemplateCommands
|
||||
updateCmd.Add(updateLockedOption);
|
||||
updateCmd.Add(updateParamsOption);
|
||||
updateCmd.Add(updateReturnOption);
|
||||
updateCmd.Add(updateMinTimeOption);
|
||||
updateCmd.Add(updateExecTimeoutOption);
|
||||
updateCmd.Add(updateScriptTriggerKindOption);
|
||||
updateCmd.SetAction(async (ParseResult result) =>
|
||||
{
|
||||
if (!TryParseMinTimeBetweenRuns(result.GetValue(updateMinTimeOption), out var minTime, out var minTimeError))
|
||||
{
|
||||
OutputFormatter.WriteError(minTimeError!, "INVALID_ARGUMENT");
|
||||
return 1;
|
||||
}
|
||||
|
||||
var rawConfig = result.GetValue(updateTriggerConfigOption);
|
||||
WarnIfTriggerKindIgnored(rawConfig, result.GetValue(updateScriptTriggerKindOption));
|
||||
var triggerConfig = TriggerConfigJson.InjectAnalysisKind(
|
||||
result.GetValue(updateTriggerConfigOption),
|
||||
rawConfig,
|
||||
result.GetValue(updateScriptTriggerKindOption));
|
||||
return await CommandHelpers.ExecuteCommandAsync(
|
||||
result, urlOption, formatOption, usernameOption, passwordOption,
|
||||
@@ -620,7 +756,9 @@ public static class TemplateCommands
|
||||
triggerConfig,
|
||||
result.GetValue(updateLockedOption),
|
||||
result.GetValue(updateParamsOption),
|
||||
result.GetValue(updateReturnOption)));
|
||||
result.GetValue(updateReturnOption),
|
||||
minTime,
|
||||
result.GetValue(updateExecTimeoutOption)));
|
||||
});
|
||||
group.Add(updateCmd);
|
||||
|
||||
|
||||
@@ -271,7 +271,7 @@ scadabridge --url <url> template alarm update --id <int> --name <string> --trigg
|
||||
| `--description` | no | Description |
|
||||
| `--trigger-config` | no | Trigger configuration JSON |
|
||||
| `--locked` | no | Lock the alarm in derived templates |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Ignored for non-`Expression` trigger types. |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Ignored for non-`Expression` trigger types. **Requires `--trigger-config`** — the kind is written into the trigger-config JSON, so on `update` it has no effect without one (the CLI prints a warning to stderr and continues). |
|
||||
|
||||
#### `template alarm delete`
|
||||
|
||||
@@ -325,7 +325,7 @@ scadabridge --url <url> template native-alarm-source remove --id <int>
|
||||
Add a script to a template.
|
||||
|
||||
```sh
|
||||
scadabridge --url <url> template script add --template-id <int> --name <string> --trigger-type <string> --code <string> [--trigger-config <json>] [--locked] [--parameters <json>] [--return-def <json>] [--trigger-kind <kind>]
|
||||
scadabridge --url <url> template script add --template-id <int> --name <string> --trigger-type <string> --code <string> [--trigger-config <json>] [--locked] [--parameters <json>] [--return-def <json>] [--min-time-between-runs <duration>] [--execution-timeout-seconds <int>] [--trigger-kind <kind>]
|
||||
```
|
||||
|
||||
| Option | Required | Description |
|
||||
@@ -338,7 +338,9 @@ scadabridge --url <url> template script add --template-id <int> --name <string>
|
||||
| `--locked` | no | Lock the script in derived templates |
|
||||
| `--parameters` | no | Parameter definitions JSON |
|
||||
| `--return-def` | no | Return definition JSON |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Only meaningful when `--trigger-type` is `Expression`. |
|
||||
| `--min-time-between-runs` | no | Minimum time between runs — a throttle for triggered scripts, or the re-fire interval for a `WhileTrue` trigger. Accepts a unit suffix `ms`, `s`/`sec`, or `m`/`min` (e.g. `500ms`, `5s`, `2min`); a bare number is seconds. Omit, or pass `0`, to leave it unset (no throttle). |
|
||||
| `--execution-timeout-seconds` | no | Per-script execution timeout in seconds. Omit (or pass a non-positive value) to use the site's global default (`SiteRuntimeOptions.ScriptExecutionTimeoutSeconds`). |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Only meaningful when `--trigger-type` is `Expression`. **Requires `--trigger-config`** — the kind is written into the trigger-config JSON, so passing `--trigger-kind` without `--trigger-config` has no effect (the CLI prints a warning to stderr and continues). |
|
||||
|
||||
#### `template script update`
|
||||
|
||||
@@ -346,7 +348,7 @@ Update a script on a template. An update **replaces** the whole entity — every
|
||||
field below must be supplied with its post-update value, even if unchanged.
|
||||
|
||||
```sh
|
||||
scadabridge --url <url> template script update --id <int> --name <string> --trigger-type <string> --code <string> [--trigger-config <json>] [--locked] [--parameters <json>] [--return-def <json>] [--trigger-kind <kind>]
|
||||
scadabridge --url <url> template script update --id <int> --name <string> --trigger-type <string> --code <string> [--trigger-config <json>] [--locked] [--parameters <json>] [--return-def <json>] [--min-time-between-runs <duration>] [--execution-timeout-seconds <int>] [--trigger-kind <kind>]
|
||||
```
|
||||
|
||||
| Option | Required | Description |
|
||||
@@ -359,7 +361,9 @@ scadabridge --url <url> template script update --id <int> --name <string> --trig
|
||||
| `--locked` | no | Lock the script in derived templates |
|
||||
| `--parameters` | no | Parameter definitions JSON |
|
||||
| `--return-def` | no | Return definition JSON |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Only meaningful when `--trigger-type` is `Expression`. |
|
||||
| `--min-time-between-runs` | no | Minimum time between runs (throttle / `WhileTrue` re-fire interval). Unit suffix `ms`, `s`/`sec`, or `m`/`min` (e.g. `500ms`, `5s`, `2min`); a bare number is seconds. Omit or `0` leaves it unset. As an update replaces the whole entity, omitting this **clears** any existing value. |
|
||||
| `--execution-timeout-seconds` | no | Per-script execution timeout in seconds; omit (or non-positive) uses the site default. As an update replaces the whole entity, omitting this **clears** any existing override. |
|
||||
| `--trigger-kind` | no | Expression-trigger analysis kind: `advisory` (default) or `strict`. Only meaningful when `--trigger-type` is `Expression`. **Requires `--trigger-config`** — without it the kind is ignored (the CLI warns to stderr and continues). |
|
||||
|
||||
#### `template script delete`
|
||||
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
using ZB.MOM.WW.ScadaBridge.CLI.Commands;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.CLI.Tests.Commands;
|
||||
|
||||
/// <summary>
|
||||
/// #257: <c>--trigger-kind</c> is written into the trigger-config JSON by
|
||||
/// <see cref="ZB.MOM.WW.ScadaBridge.CLI.TriggerConfigJson.InjectAnalysisKind"/>, which
|
||||
/// returns null (dropping the kind) when no <c>--trigger-config</c> is supplied. Passing
|
||||
/// <c>--trigger-kind</c> without <c>--trigger-config</c> was therefore a silent no-op.
|
||||
/// The CLI now detects that combination (<see cref="TemplateCommands.TriggerKindWillBeIgnored"/>)
|
||||
/// and warns the user (warn-and-continue, not a hard error). These tests pin the predicate.
|
||||
/// </summary>
|
||||
public class TemplateTriggerKindIgnoredTests
|
||||
{
|
||||
[Fact]
|
||||
public void KindWithoutConfig_WillBeIgnored()
|
||||
{
|
||||
Assert.True(TemplateCommands.TriggerKindWillBeIgnored(triggerConfig: null, triggerKind: "strict"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void KindWithBlankConfig_WillBeIgnored()
|
||||
{
|
||||
Assert.True(TemplateCommands.TriggerKindWillBeIgnored(triggerConfig: " ", triggerKind: "strict"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void KindWithConfig_NotIgnored()
|
||||
{
|
||||
Assert.False(TemplateCommands.TriggerKindWillBeIgnored(
|
||||
triggerConfig: "{\"expression\":\"x > 0\"}", triggerKind: "strict"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void NoKind_NotIgnored()
|
||||
{
|
||||
// No --trigger-kind at all → nothing to warn about, even without a config.
|
||||
Assert.False(TemplateCommands.TriggerKindWillBeIgnored(triggerConfig: null, triggerKind: null));
|
||||
Assert.False(TemplateCommands.TriggerKindWillBeIgnored(triggerConfig: null, triggerKind: " "));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void NoKindWithConfig_NotIgnored()
|
||||
{
|
||||
Assert.False(TemplateCommands.TriggerKindWillBeIgnored(
|
||||
triggerConfig: "{\"expression\":\"x > 0\"}", triggerKind: null));
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user