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

stack.Cancel support in dotnet Automation API #6729

Merged
merged 7 commits into from
Apr 13, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Apr 8, 2021

Go implementation does SelectStack first, which I think mutates workspace to current stack. Should we do the same for consistency?

Do we want CancelOptions with "OnStandardOutput" etc?

In cases where the operation fails, as in, (1) there is no pending update; (2) it's unsupported on the local workspace, do we signal these with exceptions or error codes?

Testing can be a bit tricky here let me think how to test this.

@t0yv0 t0yv0 requested a review from komalali April 8, 2021 19:48
@t0yv0 t0yv0 changed the title First cut at Cancel stack.Cancel support in dotnet Automation API Apr 8, 2021
@t0yv0
Copy link
Member Author

t0yv0 commented Apr 8, 2021

CC @orionstudt

@orionstudt
Copy link
Contributor

I think we should be doing SelectStack first just because that is how all other functionality currently works on WorkspaceStack.

As for the 2 potential errors you mentioned, are those errors the CLI will throw? Perhaps we can just allow those to be caught by CommandException?

@komalali
Copy link
Member

komalali commented Apr 8, 2021

I think we should be doing SelectStack first just because that is how all other functionality currently works on WorkspaceStack.

If we do this we'll need to remove it in the 3.0 branch since we're migrating to using the --stack flag instead of SelectStack for all operations. See https://github.com/pulumi/pulumi/pull/6415/files

@orionstudt
Copy link
Contributor

@komalali makes sense - is there a PR that already did that? If not I think we add SelectStack here and whatever PR is switching to using the --stack param would hit them all, including this one

@komalali
Copy link
Member

komalali commented Apr 8, 2021

@orionstudt yes, the PR is linked in my comment above but here it again: #6415

@orionstudt
Copy link
Contributor

Roger. Didn't realize it was already merged. Than yea just --stack

@komalali
Copy link
Member

komalali commented Apr 8, 2021

FWIW that PR (6415) actually just adds --stack to all RunCommandSync in WorkspaceStack so we don't have to add it to each individual method. And also, that PR is only merged into the 3.0 branch because it's a breaking change for the CLI.

@t0yv0
Copy link
Member Author

t0yv0 commented Apr 8, 2021

Awesome, thank you! I will finish this up tomorrow and try to cook some tests for it especially.

@t0yv0 t0yv0 marked this pull request as ready for review April 13, 2021 14:48
@t0yv0
Copy link
Member Author

t0yv0 commented Apr 13, 2021

Coming back to this. The test is not entirely satisfactory, since it will accept either Destroy or Cancel task failing, but I did not want to check in something intentionally flaky, and I'm not sure how one would make it deterministic. I have verified locally that we at least sometimes hit the branch where Cancel task succeeds in preempting Destroy.

@komalali ready for review 🙏

Copy link
Member

@komalali komalali left a comment

Choose a reason for hiding this comment

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

Nice job with the test, it's a tough one to test for sure!

@t0yv0 t0yv0 merged commit 0e2d918 into master Apr 13, 2021
@pulumi-bot pulumi-bot deleted the t0yv0/dotnet-automation-api-cancel-support branch April 13, 2021 18:09
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
* First cut at Cancel

* Add a racecond-based test for Cancel

* Auto-gen xml updates

* Fix code formatting

* Add CHANGELOG entry
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.

None yet

3 participants