Catch error for commands with output capture = false

[ivanpesin]

This is a proposed fix for inconsistency in handling exit status of command with include-command-output-in-response set to true and false.

Currently, if include-command-output-in-response: true and command exits with error code != 0, webhook will return error code 500. However, when set to false, webhook ignores errors and always returns code 200.

Moreover, there are two different paths in the code for each case, which seems as additional inconsistency.

This PR ensures command is spawned in a consistent manner and returns code 500 if command fails in both output capturing and non-capturing modes. There is no need to spawn a separate goroutine, because handler is spawned as a goroutine when the request comes in. This PR executes command in the same goroutine as the handler.

It also fixes issues we have discussed in PR #167:

• rate limit now works in both output capturing and non-capturing modes.
• removes the issue with handling channels due to additional goroutine being spawned.
• writing to non-existant channel is not possible as no additional goroutine.

Let me know if this solution works.

[adnanh]

Unfortunately, this approach will hang the request while the command is executing (even though we're not interested in it's output). That is the reason I used a separate goroutine in this case, so I can fire off the command in the background and close the request.

:-(

[ivanpesin]

Ah ok, I haven't realized you intended this way. Then I have two questions:

1. When forking command in background and not waiting for it, you always return 200, even if command has failed. Would it not make sense to add a special config parameter if you want for request return immediately? I.e. by default webhook waits for command to complete and looks at exit code, returns 200 if all ok, and 500 if command failed. If you specify something like spawn-and-return: true then it simply spawns the script and does not wait for its completion. If this makes sense to you (and I kinda like such functionality) I can get a PR ready on Saturday probably.

2. If we spawn and return request immediately, there is no easy way for us to track number of concurrent processes. The only way we can implement this is by looking at the list of processes running in the system and counting the ones that match the command we are about to start. This is unreliable though, because command can change the process name string. I would suggest that we have rate limiting applied only to cases when webhook waits for the process to complete, and ignores this parameter for entries with spawn-and-return: true.

Let me know your thoughts, I think I can get this done this Saturday if you agree to this.

[adnanh]

1. When forking command in background and not waiting for it, you always return 200, even if command has failed. Would it not make sense to add a special config parameter if you want for request return immediately? I.e. by default webhook waits for command to complete and looks at exit code, returns 200 if all ok, and 500 if command failed. If you specify something like spawn-and-return: true then it simply spawns the script and does not wait for its completion. If this makes sense to you (and I kinda like such functionality) I can get a PR ready on Saturday probably.

We actually have the opposite behavior, we have a special config parameter if you want for request to wait the script execution (include-command-output-in-response etc...), because it's used way less than the 'fire-and-forget' method.

2. If we spawn and return request immediately, there is no easy way for us to track number of concurrent processes. The only way we can implement this is by looking at the list of processes running in the system and counting the ones that match the command we are about to start. This is unreliable though, because command can change the process name string. I would suggest that we have rate limiting applied only to cases when webhook waits for the process to complete, and ignores this parameter for entries with spawn-and-return: true.

Either case would be a step forward, but maybe we could store the list of currently running hook commands and count them instead, and clean them out periodically using a separate goproc?

• add Executions field in the Hook object
• whenever the hook command should be invoked (either sync or async), if the hook has concurrency control flag set, go through the Executions array, count how many unterminated objects are in there, see if we can spawn another one and add the reference to the new os.Cmd into the Executions field)
• have a separate go proc that will periodically go through all hooks and clean up the finished executions

@ivanpesin sorry for the late reply 😂

[ivanpesin]

@adnanh Later is better than never! :) I'll take a stab at it over the weekend.