feat(python): introduce StriptTrigger and CommandsTrigger#1
Open
ayushk-sinha wants to merge 1 commit into
Open
feat(python): introduce StriptTrigger and CommandsTrigger#1ayushk-sinha wants to merge 1 commit into
ayushk-sinha wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe pull request introduces two new polling trigger classes—CommandsTrigger and ScriptTrigger—to the Kestra Python scripts plugin. Both triggers execute Python commands or scripts on a configurable interval, evaluate custom exit conditions (exit code or pattern matching), and emit events accordingly. Build dependencies are added, and comprehensive integration tests are included. Changes
Sequence DiagramsequenceDiagram
participant Scheduler
participant CommandsTrigger/ScriptTrigger as Trigger
participant TaskRunner as Task Runner
participant ConditionMatcher as Condition Matcher
participant EventQueue as Event Queue
Scheduler->>Trigger: Poll on interval
Trigger->>TaskRunner: Execute Command/Script
TaskRunner-->>Trigger: exitCode, vars, logs
Trigger->>ConditionMatcher: Match exitCondition
alt Condition Matches
ConditionMatcher-->>Trigger: true
Trigger->>EventQueue: Emit Output event
EventQueue-->>Scheduler: New Execution
else Condition Does Not Match
ConditionMatcher-->>Trigger: false
Trigger-->>Scheduler: No event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java`:
- Around line 162-166: The exit-code regex is being recompiled on every
matchesCondition call; move the Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$",
Pattern.CASE_INSENSITIVE) into a static final field (e.g., EXIT_PATTERN) in
CommandsTrigger so matchesCondition uses EXIT_PATTERN.matcher(cond) instead of
creating a new Pattern each time; update any references to exitMatcher
accordingly (keep parsing with Integer.parseInt(exitMatcher.group(1)) and the
existing exit code comparison logic).
- Around line 159-230: The duplicated logic in CommandsTrigger should be moved
into a shared component and CommandsTrigger and ScriptTrigger should use it;
extract the methods matchesCondition(Output), buildHaystack(Output),
safeExitCode(ScriptOutput), safeVars(ScriptOutput),
extractFailure(RunnableTaskException) plus the ExtractedFailure record and the
shared Output class into a common base (e.g., AbstractScriptTrigger) or a small
utility class, make them protected or package-private as appropriate, then have
CommandsTrigger and ScriptTrigger extend or delegate to that base and remove the
duplicate implementations from both classes so they reuse the single shared
implementation.
- Around line 64-74: The default container image DEFAULT_IMAGE in
CommandsTrigger should be changed from "ubuntu" to a Python-enabled image;
update the constant DEFAULT_IMAGE to "python:3.12-slim" and update the `@Schema`
description text (and any references to the default value) for the
containerImage field so it reflects the new python:3.12-slim default; ensure
Property.ofValue(DEFAULT_IMAGE) remains used so the new default is applied in
CommandsTrigger.
- Around line 28-32: Exclude the mutable field lastMatched from Lombok-generated
equals/hashCode in CommandsTrigger: update the class-level `@EqualsAndHashCode` to
exclude = {"lastMatched"} or annotate the lastMatched field with
`@EqualsAndHashCode.Exclude` so that equals/hashCode for CommandsTrigger (and
similarly for ScriptTrigger if present) do not consider the mutable lastMatched
field.
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/ScriptTrigger.java`:
- Around line 65-75: The DEFAULT_IMAGE constant in ScriptTrigger is set to
"ubuntu", which lacks Python; change DEFAULT_IMAGE to a Python-enabled image
such as "python:3.12-slim" so the Property<String> containerImage (and its
Builder.Default) defaults to a Python-capable runtime, and update the `@Schema`
description text to indicate the default uses a Python image; reference
DEFAULT_IMAGE, containerImage, and class ScriptTrigger when making these edits.
- Around line 169-177: The regex for detecting "exit N" is being recompiled on
every call to matchesCondition; make it a static final Pattern field (e.g., add
private static final Pattern EXIT_PATTERN =
Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE)) and
replace the inline Pattern.compile(...) call in matchesCondition with
EXIT_PATTERN (use EXIT_PATTERN.matcher(cond) to create exitMatcher) so the
pattern is reused and not recompiled each invocation.
- Around line 243-274: The Output inner class is currently mutable due to
Lombok's `@Data`; replace `@Data` with an immutable variant (e.g., `@Value`) or
annotate with `@Getter` and make all fields final while keeping
`@AllArgsConstructor` so no setters are generated; ensure the class signature
(public static class Output implements io.kestra.core.models.tasks.Output) and
all field names (timestamp, condition, exitCode, vars, logs) remain unchanged so
existing usages still compile.
- Around line 27-31: The mutable runtime field lastMatched in class
ScriptTrigger is currently included in Lombok's `@EqualsAndHashCode` which can
break comparisons/collections; update the annotation to exclude that field
(either change `@EqualsAndHashCode` to `@EqualsAndHashCode`(exclude = "lastMatched")
on the class or annotate the lastMatched field with `@EqualsAndHashCode.Exclude`)
so equals/hashCode only consider immutable/configuration state.
- Around line 146-167: The trigger currently builds the Script task with
Process.instance(), which ignores the containerImage; update
ScriptTrigger.runOnce to choose a container-capable runner when a container
image is provided: if this.containerImage is set/non-empty, use
Docker.instance() (or the Docker runner equivalent) in Script.builder() instead
of Process.instance(), otherwise fall back to Process.instance(); also ensure
PythonEnvironmentManager.setup() will see the container runner and that any
imports/runner types are adjusted accordingly (reference symbols:
ScriptTrigger.runOnce, this.containerImage, Script.builder(),
Process.instance(), Docker.instance(), PythonEnvironmentManager.setup()).
In
`@plugin-script-python/src/test/java/io/kestra/plugin/scripts/python/CommandsTriggerTest.java`:
- Around line 51-137: Extract the duplicated setup/wait/cleanup logic into a
shared test base or utility: move DefaultWorker creation (DefaultWorker worker =
applicationContext.createBean(...)), scheduler creation (new
JdbcScheduler(...)), flow listener spy setup (FlowListeners
flowListenersServiceSpy and doReturn(...).when(...)), the TestsUtils.receive
subscription, CountDownLatch/AtomicReference waiting pattern
(queueCount/lastExecution and Await.until usage), and shutdown/close cleanup
(worker.shutdown(), scheduler.close(), receive.blockLast()) into shared
`@Before/`@After methods or reusable helper methods; update CommandsTriggerTest
and ScriptTriggerTest to call those helpers (or extend the new base class) and
keep only trigger-specific Flow/Trigger construction and assertions in each
test. Ensure helper names are discoverable (e.g., initWorkerAndScheduler,
waitForExecution, cleanupWorkerAndScheduler) so references like DefaultWorker,
JdbcScheduler, TestsUtils.receive, Await.until, flowListenersServiceSpy,
queueCount, lastExecution are easy to find.
In
`@plugin-script-python/src/test/java/io/kestra/plugin/scripts/python/ScriptTriggerTest.java`:
- Around line 101-114: Remove the redundant Thread.sleep and the duplicated
Await.until check: rely on the CountDownLatch wait (queueCount.await(20,
TimeUnit.SECONDS)) to block until the trigger sets lastExecution; after the
latch returns, directly assert that lastExecution.get() is non-null (or use
assertThat on lastExecution) instead of calling Await.until; update the test by
deleting the Thread.sleep(Duration.ofSeconds(2).toMillis()) and the try/catch
block containing Await.until to simplify and avoid double-waiting.
Comment on lines
+28
to
+32
| @SuperBuilder | ||
| @ToString | ||
| @EqualsAndHashCode | ||
| @Getter | ||
| @NoArgsConstructor |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider excluding lastMatched from @EqualsAndHashCode.
Same concern as ScriptTrigger: the mutable lastMatched field should be excluded from equality checks.
♻️ Proposed fix
`@SuperBuilder`
`@ToString`
-@EqualsAndHashCode
+@EqualsAndHashCode(callSuper = true)
`@Getter`
`@NoArgsConstructor`And on the field (Line 117):
`@Builder.Default`
`@Getter`(AccessLevel.NONE)
+@EqualsAndHashCode.Exclude
private final AtomicBoolean lastMatched = new AtomicBoolean(false);🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java`
around lines 28 - 32, Exclude the mutable field lastMatched from
Lombok-generated equals/hashCode in CommandsTrigger: update the class-level
`@EqualsAndHashCode` to exclude = {"lastMatched"} or annotate the lastMatched
field with `@EqualsAndHashCode.Exclude` so that equals/hashCode for
CommandsTrigger (and similarly for ScriptTrigger if present) do not consider the
mutable lastMatched field.
Comment on lines
+64
to
+74
| private static final String DEFAULT_IMAGE = "ubuntu"; | ||
|
|
||
| @Schema( | ||
| title = "Docker image used to execute the commands.", | ||
| description = """ | ||
| Container image used by the underlying Commands task to run shell commands. | ||
| Defaults to 'ubuntu'. | ||
| """ | ||
| ) | ||
| @Builder.Default | ||
| protected Property<String> containerImage = Property.ofValue(DEFAULT_IMAGE); |
There was a problem hiding this comment.
Default container image lacks Python.
Same issue as ScriptTrigger: the default ubuntu image does not include Python. Consider using python:3.12-slim as the default for consistency with the Python commands purpose.
🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java`
around lines 64 - 74, The default container image DEFAULT_IMAGE in
CommandsTrigger should be changed from "ubuntu" to a Python-enabled image;
update the constant DEFAULT_IMAGE to "python:3.12-slim" and update the `@Schema`
description text (and any references to the default value) for the
containerImage field so it reflects the new python:3.12-slim default; ensure
Property.ofValue(DEFAULT_IMAGE) remains used so the new default is applied in
CommandsTrigger.
Comment on lines
+159
to
+230
| private boolean matchesCondition(Output out) { | ||
| String cond = out.getCondition() == null ? "" : out.getCondition().trim(); | ||
|
|
||
| Matcher exitMatcher = Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE).matcher(cond); | ||
| if (exitMatcher.matches()) { | ||
| int expected = Integer.parseInt(exitMatcher.group(1)); | ||
| return out.getExitCode() != null && out.getExitCode() == expected; | ||
| } | ||
|
|
||
| String haystack = buildHaystack(out); | ||
| if (haystack.isEmpty() || cond.isEmpty()) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return Pattern.compile(cond).matcher(haystack).find(); | ||
| } catch (Exception invalidRegex) { | ||
| return haystack.contains(cond); | ||
| } | ||
| } | ||
|
|
||
| private String buildHaystack(Output out) { | ||
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
| if (out.getVars() != null && !out.getVars().isEmpty()) { | ||
| sb.append(out.getVars()).append("\n"); | ||
| } | ||
| if (out.getLogs() != null && !out.getLogs().isBlank()) { | ||
| sb.append(out.getLogs()).append("\n"); | ||
| } | ||
|
|
||
| return sb.toString(); | ||
| } | ||
|
|
||
| private Integer safeExitCode(ScriptOutput taskOutput) { | ||
| try { | ||
| return taskOutput.getExitCode(); | ||
| } catch (Exception ignored) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private Map<String, Object> safeVars(ScriptOutput taskOutput) { | ||
| try { | ||
| return taskOutput.getVars(); | ||
| } catch (Exception ignored) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private record ExtractedFailure(Integer exitCode, String logs) { | ||
| } | ||
|
|
||
| private ExtractedFailure extractFailure(RunnableTaskException e) { | ||
| Integer exitCode = null; | ||
| String logs = null; | ||
|
|
||
| Throwable cur = e.getCause(); | ||
| while (cur != null) { | ||
| if (cur instanceof TaskException te) { | ||
| exitCode = te.getExitCode(); | ||
| try { | ||
| logs = te.getLogConsumer() != null ? te.getLogConsumer().toString() : null; | ||
| } catch (Exception ignored) { | ||
| } | ||
| break; | ||
| } | ||
| cur = cur.getCause(); | ||
| } | ||
|
|
||
| return new ExtractedFailure(exitCode, logs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication with ScriptTrigger.
The matchesCondition, buildHaystack, safeExitCode, safeVars, extractFailure methods, the ExtractedFailure record, and the Output class are nearly identical between CommandsTrigger and ScriptTrigger. Consider extracting these into a shared base class (e.g., AbstractScriptTrigger) or a utility class to reduce duplication and simplify future maintenance.
🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java`
around lines 159 - 230, The duplicated logic in CommandsTrigger should be moved
into a shared component and CommandsTrigger and ScriptTrigger should use it;
extract the methods matchesCondition(Output), buildHaystack(Output),
safeExitCode(ScriptOutput), safeVars(ScriptOutput),
extractFailure(RunnableTaskException) plus the ExtractedFailure record and the
shared Output class into a common base (e.g., AbstractScriptTrigger) or a small
utility class, make them protected or package-private as appropriate, then have
CommandsTrigger and ScriptTrigger extend or delegate to that base and remove the
duplicate implementations from both classes so they reuse the single shared
implementation.
Comment on lines
+162
to
+166
| Matcher exitMatcher = Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE).matcher(cond); | ||
| if (exitMatcher.matches()) { | ||
| int expected = Integer.parseInt(exitMatcher.group(1)); | ||
| return out.getExitCode() != null && out.getExitCode() == expected; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching the exit-code pattern.
Same as ScriptTrigger: the exit pattern should be a static final field rather than compiled on every matchesCondition call.
🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java`
around lines 162 - 166, The exit-code regex is being recompiled on every
matchesCondition call; move the Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$",
Pattern.CASE_INSENSITIVE) into a static final field (e.g., EXIT_PATTERN) in
CommandsTrigger so matchesCondition uses EXIT_PATTERN.matcher(cond) instead of
creating a new Pattern each time; update any references to exitMatcher
accordingly (keep parsing with Integer.parseInt(exitMatcher.group(1)) and the
existing exit code comparison logic).
Comment on lines
+27
to
+31
| @SuperBuilder | ||
| @ToString | ||
| @EqualsAndHashCode | ||
| @Getter | ||
| @NoArgsConstructor |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider excluding lastMatched from @EqualsAndHashCode.
The lastMatched field is mutable runtime state (an AtomicBoolean), not configuration. Including it in equals/hashCode can cause unexpected behavior when comparing trigger instances or using them in collections.
♻️ Proposed fix
`@SuperBuilder`
`@ToString`
-@EqualsAndHashCode
+@EqualsAndHashCode(callSuper = true)
`@Getter`
`@NoArgsConstructor`And add exclusion on the field:
`@Builder.Default`
`@Getter`(AccessLevel.NONE)
+@EqualsAndHashCode.Exclude
private final AtomicBoolean lastMatched = new AtomicBoolean(false);🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/ScriptTrigger.java`
around lines 27 - 31, The mutable runtime field lastMatched in class
ScriptTrigger is currently included in Lombok's `@EqualsAndHashCode` which can
break comparisons/collections; update the annotation to exclude that field
(either change `@EqualsAndHashCode` to `@EqualsAndHashCode`(exclude = "lastMatched")
on the class or annotate the lastMatched field with `@EqualsAndHashCode.Exclude`)
so equals/hashCode only consider immutable/configuration state.
Comment on lines
+169
to
+177
| private boolean matchesCondition(Output out) { | ||
| String cond = out.getCondition() == null ? "" : out.getCondition().trim(); | ||
|
|
||
| // 1) exit N | ||
| Matcher exitMatcher = Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE).matcher(cond); | ||
| if (exitMatcher.matches()) { | ||
| int expected = Integer.parseInt(exitMatcher.group(1)); | ||
| return out.getExitCode() != null && out.getExitCode() == expected; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching the exit-code pattern.
The regex Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE) is compiled on every invocation of matchesCondition. Since this pattern is constant, it should be a static final field for efficiency.
♻️ Proposed fix
Add a static field:
private static final Pattern EXIT_PATTERN = Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE);Then use it in matchesCondition:
- Matcher exitMatcher = Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE).matcher(cond);
+ Matcher exitMatcher = EXIT_PATTERN.matcher(cond);🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/ScriptTrigger.java`
around lines 169 - 177, The regex for detecting "exit N" is being recompiled on
every call to matchesCondition; make it a static final Pattern field (e.g., add
private static final Pattern EXIT_PATTERN =
Pattern.compile("^\\s*exit\\s+(\\d+)\\s*$", Pattern.CASE_INSENSITIVE)) and
replace the inline Pattern.compile(...) call in matchesCondition with
EXIT_PATTERN (use EXIT_PATTERN.matcher(cond) to create exitMatcher) so the
pattern is reused and not recompiled each invocation.
Comment on lines
+243
to
+274
| @Data | ||
| @AllArgsConstructor | ||
| public static class Output implements io.kestra.core.models.tasks.Output { | ||
| private Instant timestamp; | ||
|
|
||
| @Schema( | ||
| title = "Rendered condition.", | ||
| description = "Rendered value of the exitCondition property for this poll." | ||
| ) | ||
| private String condition; | ||
|
|
||
| @Schema( | ||
| title = "Script exit code.", | ||
| description = "Exit code returned by the shell process (may be null if not available)." | ||
| ) | ||
| private Integer exitCode; | ||
|
|
||
| @Schema( | ||
| title = "Script vars.", | ||
| description = """ | ||
| Vars produced by the task (e.g. via ::{"outputs":{...}}:: convention). This is the main structured | ||
| way to evaluate non-exit conditions on successful runs. | ||
| """ | ||
| ) | ||
| private Map<String, Object> vars; | ||
|
|
||
| @Schema( | ||
| title = "Captured logs (best effort).", | ||
| description = "Captured error logs when the script fails (best effort, depends on the runner)." | ||
| ) | ||
| private String logs; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making Output immutable.
Using @Data generates setters, making the output mutable. Trigger outputs are typically immutable once created. Consider using @Value (or @Getter with final fields) for better immutability guarantees.
♻️ Proposed fix
-@Data
-@AllArgsConstructor
+@Value
public static class Output implements io.kestra.core.models.tasks.Output {
- private Instant timestamp;
+ Instant timestamp;
// ... other fields similarly
}Or keep @AllArgsConstructor with @Getter only:
-@Data
+@Getter
`@AllArgsConstructor`
public static class Output implements io.kestra.core.models.tasks.Output {
+ private final Instant timestamp;
// ... make other fields final
}🤖 Prompt for AI Agents
In
`@plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/ScriptTrigger.java`
around lines 243 - 274, The Output inner class is currently mutable due to
Lombok's `@Data`; replace `@Data` with an immutable variant (e.g., `@Value`) or
annotate with `@Getter` and make all fields final while keeping
`@AllArgsConstructor` so no setters are generated; ensure the class signature
(public static class Output implements io.kestra.core.models.tasks.Output) and
all field names (timestamp, condition, exitCode, vars, logs) remain unchanged so
existing usages still compile.
Comment on lines
+51
to
+137
| @Test | ||
| void commandsTrigger_shouldTriggerOnImplicitFailureExit1() throws Exception { | ||
| FlowListeners flowListenersServiceSpy = spy(this.flowListenersService); | ||
|
|
||
| CommandsTrigger trigger = CommandsTrigger.builder() | ||
| .id("commands-trigger") | ||
| .type(CommandsTrigger.class.getName()) | ||
| .interval(Duration.ofSeconds(1)) | ||
| .exitCondition(Property.ofValue("exit 1")) | ||
| .edge(Property.ofValue(true)) | ||
| .containerImage(Property.ofValue("python:3.12-slim")) | ||
| .commands(Property.ofValue(List.of( | ||
| // Implicit failure -> non-zero exit code | ||
| "python -c \"import sys; sys.exit(1)\"" | ||
| ))) | ||
| .build(); | ||
|
|
||
| Flow testFlow = Flow.builder() | ||
| .id("commands-trigger-flow") | ||
| .namespace("io.kestra.tests") | ||
| .revision(1) | ||
| .tasks(Collections.singletonList(Return.builder() | ||
| .id("log-trigger-vars") | ||
| .type(Return.class.getName()) | ||
| .format(Property.ofValue("exitCode={{ trigger.exitCode }}, condition={{ trigger.condition }}")) | ||
| .build())) | ||
| .triggers(Collections.singletonList(trigger)) | ||
| .build(); | ||
|
|
||
| FlowWithSource flow = FlowWithSource.of(testFlow, null); | ||
| doReturn(List.of(flow)).when(flowListenersServiceSpy).flows(); | ||
|
|
||
| CountDownLatch queueCount = new CountDownLatch(1); | ||
| AtomicReference<Execution> lastExecution = new AtomicReference<>(); | ||
|
|
||
| Flux<Execution> receive = TestsUtils.receive(executionQueue, execution -> { | ||
| if (execution.getLeft().getFlowId().equals("commands-trigger-flow")) { | ||
| lastExecution.set(execution.getLeft()); | ||
| queueCount.countDown(); | ||
| } | ||
| }); | ||
|
|
||
| DefaultWorker worker = applicationContext.createBean(DefaultWorker.class, IdUtils.create(), 8, null); | ||
| AbstractScheduler scheduler = new JdbcScheduler(applicationContext, flowListenersServiceSpy); | ||
|
|
||
| try { | ||
| worker.run(); | ||
| scheduler.run(); | ||
|
|
||
| Thread.sleep(Duration.ofSeconds(2).toMillis()); | ||
|
|
||
| boolean await = queueCount.await(20, TimeUnit.SECONDS); | ||
| assertThat("CommandsTrigger should execute", await, is(true)); | ||
|
|
||
| try { | ||
| Await.until( | ||
| () -> lastExecution.get() != null, | ||
| Duration.ofMillis(100), | ||
| Duration.ofSeconds(2) | ||
| ); | ||
| } catch (TimeoutException e) { | ||
| throw new AssertionError("Execution was not captured within 2 seconds", e); | ||
| } | ||
|
|
||
| Execution execution = lastExecution.get(); | ||
| assertThat(execution, notNullValue()); | ||
|
|
||
| Map<String, Object> triggerVars = execution.getTrigger().getVariables(); | ||
| assertThat("condition should be present", triggerVars.get("condition"), is("exit 1")); | ||
| assertThat("exitCode should be present", triggerVars.get("exitCode"), notNullValue()); | ||
| assertThat("exitCode should be 1", triggerVars.get("exitCode"), is(1)); | ||
| assertThat("timestamp should be present", triggerVars.get("timestamp"), notNullValue()); | ||
| } finally { | ||
| try { | ||
| worker.shutdown(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| try { | ||
| scheduler.close(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| try { | ||
| receive.blockLast(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test structure largely duplicates ScriptTriggerTest.
Both test classes share nearly identical setup, waiting logic, and cleanup patterns. Consider extracting common test infrastructure (worker/scheduler setup, execution waiting, cleanup) into a shared base class or utility to reduce maintenance burden.
🤖 Prompt for AI Agents
In
`@plugin-script-python/src/test/java/io/kestra/plugin/scripts/python/CommandsTriggerTest.java`
around lines 51 - 137, Extract the duplicated setup/wait/cleanup logic into a
shared test base or utility: move DefaultWorker creation (DefaultWorker worker =
applicationContext.createBean(...)), scheduler creation (new
JdbcScheduler(...)), flow listener spy setup (FlowListeners
flowListenersServiceSpy and doReturn(...).when(...)), the TestsUtils.receive
subscription, CountDownLatch/AtomicReference waiting pattern
(queueCount/lastExecution and Await.until usage), and shutdown/close cleanup
(worker.shutdown(), scheduler.close(), receive.blockLast()) into shared
`@Before/`@After methods or reusable helper methods; update CommandsTriggerTest
and ScriptTriggerTest to call those helpers (or extend the new base class) and
keep only trigger-specific Flow/Trigger construction and assertions in each
test. Ensure helper names are discoverable (e.g., initWorkerAndScheduler,
waitForExecution, cleanupWorkerAndScheduler) so references like DefaultWorker,
JdbcScheduler, TestsUtils.receive, Await.until, flowListenersServiceSpy,
queueCount, lastExecution are easy to find.
Comment on lines
+101
to
+114
| Thread.sleep(Duration.ofSeconds(2).toMillis()); | ||
|
|
||
| boolean await = queueCount.await(20, TimeUnit.SECONDS); | ||
| assertThat("ScriptTrigger should execute", await, is(true)); | ||
|
|
||
| try { | ||
| Await.until( | ||
| () -> lastExecution.get() != null, | ||
| Duration.ofMillis(100), | ||
| Duration.ofSeconds(2) | ||
| ); | ||
| } catch (TimeoutException e) { | ||
| throw new AssertionError("Execution was not captured within 2 seconds", e); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant waiting logic.
The Thread.sleep(2s) before queueCount.await(20s) is unnecessary since the latch already waits. Additionally, the Await.until check (Lines 106-114) is redundant because queueCount.await already ensures the execution was received (the latch only counts down after lastExecution.set).
♻️ Simplified waiting
- Thread.sleep(Duration.ofSeconds(2).toMillis());
-
boolean await = queueCount.await(20, TimeUnit.SECONDS);
assertThat("ScriptTrigger should execute", await, is(true));
- try {
- Await.until(
- () -> lastExecution.get() != null,
- Duration.ofMillis(100),
- Duration.ofSeconds(2)
- );
- } catch (TimeoutException e) {
- throw new AssertionError("Execution was not captured within 2 seconds", e);
- }
-
Execution execution = lastExecution.get();
+ assertThat(execution, notNullValue());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Thread.sleep(Duration.ofSeconds(2).toMillis()); | |
| boolean await = queueCount.await(20, TimeUnit.SECONDS); | |
| assertThat("ScriptTrigger should execute", await, is(true)); | |
| try { | |
| Await.until( | |
| () -> lastExecution.get() != null, | |
| Duration.ofMillis(100), | |
| Duration.ofSeconds(2) | |
| ); | |
| } catch (TimeoutException e) { | |
| throw new AssertionError("Execution was not captured within 2 seconds", e); | |
| } | |
| boolean await = queueCount.await(20, TimeUnit.SECONDS); | |
| assertThat("ScriptTrigger should execute", await, is(true)); | |
| Execution execution = lastExecution.get(); | |
| assertThat(execution, notNullValue()); |
🤖 Prompt for AI Agents
In
`@plugin-script-python/src/test/java/io/kestra/plugin/scripts/python/ScriptTriggerTest.java`
around lines 101 - 114, Remove the redundant Thread.sleep and the duplicated
Await.until check: rely on the CountDownLatch wait (queueCount.await(20,
TimeUnit.SECONDS)) to block until the trigger sets lastExecution; after the
latch returns, directly assert that lastExecution.get() is non-null (or use
assertThat on lastExecution) instead of calling Await.until; update the test by
deleting the Thread.sleep(Duration.ofSeconds(2).toMillis()) and the try/catch
block containing Await.until to simplify and avoid double-waiting.
|
Note Docstrings generation - SKIPPED |
coderabbitai Bot
added a commit
that referenced
this pull request
Jan 26, 2026
Docstrings generation was requested by @ayushk-sinha. * #1 (comment) The following files were modified: * `plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/CommandsTrigger.java` * `plugin-script-python/src/main/java/io/kestra/plugin/scripts/python/ScriptTrigger.java`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add Python
CommandsTrigger&ScriptTrigger+ matching testsio.kestra.plugin.scripts.python.CommandsTriggerAdded
io.kestra.plugin.scripts.python.ScriptTriggerAdded Python test.
How to test?
./gradlew :plugin-script-python:testSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.