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

Support canceling remote commands via context. (#1) #128

Merged
merged 2 commits into from
Sep 17, 2022

Conversation

kgadams
Copy link
Contributor

@kgadams kgadams commented Jun 7, 2022

This solves the issue #125.

Copy link
Owner

@masterzen masterzen left a comment

Choose a reason for hiding this comment

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

Looks good to me,
Thanks for your contribution !

close(command.done)
return
case <-ctxDone:
Copy link
Owner

Choose a reason for hiding this comment

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

you can write:

case <-ctx.Done():
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an optimization. We don't exit the loop here, so this would fire every iteration through the loop. With the code as it is, it only fires a single time, then we set ctxDone to nil and Close() the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now it really does what I intended it to do after I pulled the definition of ctxDone out of the loop.

@masterzen
Copy link
Owner

Would you mind adding a section in the README to add minimal documentation regarding the option of passing a context to the various functions?

Run linter and fix linter issues.
@kgadams
Copy link
Contributor Author

kgadams commented Sep 13, 2022

Hello Brice, did you have something different in mind? Anything missing? Thanks...

@masterzen
Copy link
Owner

Hello Brice, did you have something different in mind? Anything missing? Thanks...

Hi, sorry for my lack of answers, I was abroad with limited Internet access.
That seems very fine to me ;)

Copy link
Owner

@masterzen masterzen left a comment

Choose a reason for hiding this comment

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

LGTM 🍾

@masterzen masterzen merged commit b07f6cb into masterzen:master Sep 17, 2022
kgadams added a commit to kgadams/winrm that referenced this pull request Dec 12, 2022
Merge pull request masterzen#128 from kgadams/master
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