-
-
Notifications
You must be signed in to change notification settings - Fork 780
Execution Graph to detect cyclic tasks #2287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@korverdev Would you like to try this for you use case mentioned in comments of #820? Clone the PR and then run task like this: |
|
I can confirm that this PR also fixes #2302 |
| CodeTaskInternal | ||
| CodeTaskNameConflict | ||
| CodeTaskCalledTooManyTimes | ||
| CodeTaskCalledTooManyTimes // Depreciated: replaced by CodeTaskCyclicExecutionDetected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling:
| CodeTaskCalledTooManyTimes // Depreciated: replaced by CodeTaskCyclicExecutionDetected. | |
| CodeTaskCalledTooManyTimes // Deprecated: replaced by CodeTaskCyclicExecutionDetected. |
| func (err *TaskCyclicExecutionDetectedError) Error() string { | ||
| if len(err.CallingTaskName) > 0 { | ||
| return fmt.Sprintf( | ||
| `task: Cyclic task call execution detected for task %q (calling task %q)`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd simplify the wording a little: task: Call cycle detected for task %q (calling task %q)
| ) | ||
| } else { | ||
| return fmt.Sprintf( | ||
| `task: Cyclic task call execution detected for task %q`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: task: Call cycle detected for task %q.
| CodeTaskCancelled | ||
| CodeTaskMissingRequiredVars | ||
| CodeTaskNotAllowedVars | ||
| CodeTaskCyclicExecutionDetected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might CodeTaskCallCycleDetected be a better name?
| // TaskCyclicExecutionDetectedError is returned when the Execution Graph detects | ||
| // a cyclic execution condition. | ||
| type TaskCyclicExecutionDetectedError struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I'd use CallCycle instead of CyclicExecution everywhere:
| // TaskCyclicExecutionDetectedError is returned when the Execution Graph detects | |
| // a cyclic execution condition. | |
| type TaskCyclicExecutionDetectedError struct { | |
| // TaskCallCycleDetectedError is returned when the Execution Graph detects | |
| // a call cycle. | |
| type TaskCallCycleDetectedError struct { |
|
This PR also fixes #2546 |
|
@trulede Sorry about the late follow up. Last year got really crazy for me. I'm afraid I can't test this on my example. We had to pull the trigger on replacing task with another tool due to this limitation. Is there anything I can do to get this over the line? |
|
@trulede Mind rebasing this PR? |
Execution graph to detect cyclic tasks, replacing the TaskCalledTooManyTimesError mechanism.
Vertices in the graph are added for each unique call (to a task) and the PreventCycles feature of the graph is used to detect cyclic execution. The unique identifying properties are:
Although this method has worked remarkably well, it could be beneficial to have a mechanism to disable the detection: Environment variable (of course) and perhaps attached to an existing CLI parameter (--force).
fixes #820
fixes #2302