fix(core): enhance validation for continuous task dependencies #31786
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
View your CI Pipeline Execution ↗ for commit 5f5fbe2
☁️ Nx Cloud last updated this comment at |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| for (const dependency of taskGraph.continuousDependencies[task.id]) { | ||
| if (taskGraph.tasks[dependency].parallelism === false) { | ||
| nonParallelContinousTasksThatAreDependedOn.push( | ||
| taskGraph.tasks[dependency] | ||
| ); | ||
| } |
There was a problem hiding this comment.
The loop checking for non-parallel continuous tasks is nested inside the previous conditional block, which means it only executes when the current task has parallelism === false and has continuous dependencies. This will miss scenarios where a task with parallelism === true depends on a continuous task with parallelism === false.
To catch all cases where a continuous task with parallelism === false is depended on, this loop should be moved outside the previous if statement but kept within the main for loop that iterates through all tasks.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -159,19 +159,33 @@ export function validateNoAtomizedTasks( | |||
| export function assertTaskGraphDoesNotContainInvalidTargets( | |||
| taskGraph: TaskGraph | |||
| ) { | |||
| const invalidTasks = []; | |||
| const nonParallelTasksThatDependOnContinuousTasks = []; | |||
| const nonParallelContinuousTasksThatAreDependedOn = []; | |||
There was a problem hiding this comment.
There's a typo in the variable name nonParallelContinousTasksThatAreDependedOn (missing the letter 'u' in "Continuous"). This misspelling appears in multiple places in the file. The correct spelling should be nonParallelContinuousTasksThatAreDependedOn.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
AgentEnder
force-pushed
the
fix/continuous-deps-error
branch
from
4c8e4e8 to
8b961d8
Compare
AgentEnder
force-pushed
the
fix/continuous-deps-error
branch
from
8b961d8 to
17ea421
Compare
| @@ -1,11 +1,14 @@ | |||
| import exp = require('node:constants'); | |||
| ); | ||
| } | ||
| if (nonParallelContinuousTasksThatAreDependedOn.length > 0) { | ||
| throw new NonParallelContinuousTaskIsDependedOnError( |
There was a problem hiding this comment.
| throw new NonParallelContinuousTaskIsDependedOnError( | |
| throw new DependingOnNonParallelContinuousTaskError( |
Improve assertTaskGraphDoesNotContainInvalidTargets to validate two scenarios: - Non-parallel tasks that depend on continuous tasks - Non-parallel continuous tasks that are depended on by other tasks Signed-off-by: AgentEnder <[email protected]>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
AgentEnder
force-pushed
the
fix/continuous-deps-error
branch
from
17ea421 to
5f5fbe2
Compare
## Current Behavior We error if a non-parallel task has a continuous dependency, but when running a command that contains a task that depends on a continuous task which itself is marked as non-parallel we do not error. This results in the command hanging on the continuous non-parallel task, instead of running the parent task as well. ## Expected Behavior We error in both situations. ## Related Issue(s) <!-- Please link the issue being fixed so it gets closed when this is merged. --> Fixes # --------- Signed-off-by: AgentEnder <[email protected]> Co-authored-by: Jason Jean <[email protected]> Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Co-authored-by: nx-cloud[bot] <71083854+nx-cloud[bot]@users.noreply.github.com> (cherry picked from commit 4ac6923)
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
We error if a non-parallel task has a continuous dependency, but when running a command that contains a task that depends on a continuous task which itself is marked as non-parallel we do not error. This results in the command hanging on the continuous non-parallel task, instead of running the parent task as well.
Expected Behavior
We error in both situations.
Related Issue(s)
Fixes #