Skip to content

Fix Timeout-minutes for Whole Composite Action Step - #599

Merged
ethanchewy merged 10 commits into
mainfrom
users/ethanchewy/fixCompositeTimeout
Jul 22, 2020
Merged

Fix Timeout-minutes for Whole Composite Action Step#599
ethanchewy merged 10 commits into
mainfrom
users/ethanchewy/fixCompositeTimeout

Conversation

@ethanchewy

@ethanchewy ethanchewy commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

The timeout-minutes token set in the workflow file for an individual composite action is correctly processed with the CancellationToken for the ExecutionContext of the whole composite action step but for some reason my composite action cannot differentiate between cancellation and timing out

In this PR, we fix that. We also remove unnecessary code as well.

Example Cancel: https://github.com/ethanchewy/testing-actions/runs/891636687
Example of 1 min. Timeout: https://github.com/ethanchewy/testing-actions/runs/891641094?check_suite_focus=true

@ethanchewy
ethanchewy marked this pull request as draft July 17, 2020 20:34
}

// Fixup the step result if ContinueOnError.
if (step.ExecutionContext.Result == TaskResult.Failed)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this since we'll explicitly disallow Continue on error for each composite step for now.

Comment thread src/Runner.Worker/StepsRunner.cs Outdated
Comment thread src/Runner.Worker/Handlers/CompositeActionHandler.cs Outdated
@ethanchewy
ethanchewy marked this pull request as ready for review July 20, 2020 17:12
@ethanchewy
ethanchewy requested a review from ericsciple July 20, 2020 17:13
Comment thread src/Runner.Worker/ExecutionContext.cs Outdated

bool EchoOnActionCommand { get; set; }

IExecutionContext JobExecutionContext { get; set; }

@ericsciple ericsciple Jul 22, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we expose Root { get; } instead?

Comment thread src/Runner.Worker/ExecutionContext.cs Outdated
Dictionary<string, string> envData)
{
step.ExecutionContext = Root.CreateChild(_record.Id, step.DisplayName, _record.Id.ToString("N"), scopeName, step.Action.ContextName, logger: _logger);
step.ExecutionContext = Root.CreateChild(_record.Id, step.DisplayName, _record.Id.ToString("N"), scopeName, step.Action.ContextName, logger: _logger, cancellationTokenSource: _cancellationTokenSource);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense for the interface to pass CancellationToken.CreateLinkedTokenSource(_cancellationTokenSource)

@ethanchewy
ethanchewy merged commit d5a5550 into main Jul 22, 2020
fabianishere pushed a commit to fabianishere/runner that referenced this pull request Aug 24, 2020
* Exploring child Linked Cancellation Tokens

* Preliminary Timeout-minutes fix

* Final Solution for resolving cancellation token's timeout vs. cancellation

* Clean up + Fix error handling

* Use linked tokens instead

* Clean up

* one liner

* Remove JobExecutionContext => Replace with public Root accessor

* Move CreateLinkedTokenSource in the CreateCompositeStep Function
fabianishere pushed a commit to fabianishere/runner that referenced this pull request Sep 23, 2020
* Exploring child Linked Cancellation Tokens

* Preliminary Timeout-minutes fix

* Final Solution for resolving cancellation token's timeout vs. cancellation

* Clean up + Fix error handling

* Use linked tokens instead

* Clean up

* one liner

* Remove JobExecutionContext => Replace with public Root accessor

* Move CreateLinkedTokenSource in the CreateCompositeStep Function
AdamOlech pushed a commit to antmicro/runner that referenced this pull request Jan 28, 2021
* Exploring child Linked Cancellation Tokens

* Preliminary Timeout-minutes fix

* Final Solution for resolving cancellation token's timeout vs. cancellation

* Clean up + Fix error handling

* Use linked tokens instead

* Clean up

* one liner

* Remove JobExecutionContext => Replace with public Root accessor

* Move CreateLinkedTokenSource in the CreateCompositeStep Function
TingluoHuang pushed a commit that referenced this pull request Apr 21, 2021
* Exploring child Linked Cancellation Tokens

* Preliminary Timeout-minutes fix

* Final Solution for resolving cancellation token's timeout vs. cancellation

* Clean up + Fix error handling

* Use linked tokens instead

* Clean up

* one liner

* Remove JobExecutionContext => Replace with public Root accessor

* Move CreateLinkedTokenSource in the CreateCompositeStep Function
@TingluoHuang
TingluoHuang deleted the users/ethanchewy/fixCompositeTimeout branch September 1, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants