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

feat: run command with context #95

Merged
merged 2 commits into from
May 10, 2023
Merged

feat: run command with context #95

merged 2 commits into from
May 10, 2023

Conversation

aymanbagabas
Copy link
Contributor

Pass a context down to *exec.Command and make timing out optional when o.Timeout < 0.

Using context.Background() all the time is not practical as sometimes we need to stop the execution when a parent context stops.

Describe the pull request

A clear and concise description of what the pull request is about, i.e. what problem should be fixed?

Link to the issue:

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code.

Pass a context down to *exec.Command and make timing out optional when
o.Timeout < 0.

Using `context.Background()` _all the time_ is not practical as
sometimes we need to stop the execution when _a_ parent context stops.

Signed-off-by: Ayman Bagabas <[email protected]>
@aymanbagabas
Copy link
Contributor Author

@unknwon let me know what you think

@unknwon
Copy link
Member

unknwon commented May 9, 2023

@unknwon let me know what you think

Thanks, I think it should be great, but I would need to double check https://github.com/gogs/gogs because some places might have just assumed -1 is default timeout.

@aymanbagabas
Copy link
Contributor Author

I've made some additional changes and deprecated the old options timeout in favor of using CommandOptions.Timeout

CommandOptions now can take a context and a timeout. Deprecate current
options timeout property in favor of that.

The current behavior is not affected and RunWithTimeout methods should
be removed in the future.

Use *Command.WithTimeout and *Command.SetTimeout instead.

Signed-off-by: Ayman Bagabas <[email protected]>
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #95 (975c24c) into master (2085e8a) will decrease coverage by 0.28%.
The diff coverage is 83.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   83.68%   83.41%   -0.28%     
==========================================
  Files          28       28              
  Lines        1784     1809      +25     
==========================================
+ Hits         1493     1509      +16     
- Misses        176      182       +6     
- Partials      115      118       +3     

Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Thank you!

@unknwon unknwon merged commit 02ff2f4 into gogs:master May 10, 2023
@unknwon
Copy link
Member

unknwon commented May 10, 2023

https://github.com/gogs/git-module/releases/tag/v1.8.2 has been created for this merge.

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