Skip to content
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

Fully enable shutdown for plan and refresh in the local backend #16833

Merged
merged 3 commits into from
Dec 5, 2017

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Dec 4, 2017

The backends need to specifically watch for and handle context cancellation because the context received for the operation is disjoint from the context used in the RunningOperation. This was only implemented in the apply operation, so the other operations couldn't be fully cancelled.

@jbardin
Copy link
Member Author

jbardin commented Dec 4, 2017

This is an addendum to #16816, which enabled the cancellation in the commands, but expected the backend cancellation to work.

Cancellation in the local backend was only implemented for apply.
The mock provider couldn't be stopped during diff, because the single
mutex was held through the oepration. Release the mutex so Stop can be
called.
Now that the local backend can be cancelled during plan and refresh, we
don't really need the testShutdownHook. Simplify the tests by just
checking for Stop being called on the provider.
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Just like the last one, I feel a bit uneasy about how much code is duplicated here but making this work properly is the important thing and we can revisit and restructure this later.

@jbardin
Copy link
Member Author

jbardin commented Dec 5, 2017

I agree. Trying to DRY those though will take some work on better abstractions, singe all three operations operate on slightly different types, with slightly different UI output, and different checks after.

@jbardin jbardin merged commit 12b7dac into master Dec 5, 2017
@jbardin jbardin deleted the jbardin/plan-shutdown branch December 5, 2017 21:48
@ghost
Copy link

ghost commented Apr 5, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants