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

Add an option of timeout duration for executing command #460

Merged
merged 5 commits into from Jan 6, 2018

Conversation

taku-k
Copy link
Contributor

@taku-k taku-k commented Dec 28, 2017

What

This pull-request introduces the option for specifying a duration after which a command execution will be timeout.

Why

Running a slightly heavy check command will be easily timeout, so users should be able to adjust the timeout duration.

@Songmu
Copy link
Contributor

Songmu commented Jan 5, 2018

That's a good pull request! We wanted to implement it for a long time.

I point out two points.

Option name

I think timeout_seconds would be better.

naming context is confusing

I think it is better not to use context in naming except for the standard context package
in Go world.

How about naming it as CommandOption and adding it to the end of the argument?

(In the first place, the package name util is not good itself. There is room for
improvement there.)

@taku-k
Copy link
Contributor Author

taku-k commented Jan 6, 2018

Thank you for the review. I followed your advice.

@Songmu
Copy link
Contributor

Songmu commented Jan 6, 2018

Thanks for quick fixing.

One more. You should set timeoutDururation for action in buildCheckPlugin.

@taku-k
Copy link
Contributor Author

taku-k commented Jan 6, 2018

I miss it. Thank you.

@Songmu
Copy link
Contributor

Songmu commented Jan 6, 2018

Thanks! I

@Songmu Songmu merged commit fb24a7e into mackerelio:master Jan 6, 2018
@taku-k taku-k deleted the add-option-cmd-timeout branch January 7, 2018 06:32
@astj astj mentioned this pull request Jan 10, 2018
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

2 participants