search

grafana / xk6-exec

A k6 extension for running external commands.
Apache License 2.0
20 stars 21 forks source link

Output when execution fails #12

Open filhodanuvem opened 1 year ago

filhodanuvem commented 1 year ago

I think it would be helpful to have a way to see/get the output of failure executions, from what I see we don't have it because the log.fatal here exits the run. What do you think?

javaducky commented 1 year ago

Hi @filhodanuvem! Thank you for noticing that...it's not much help when the command silently fails.

diegombeltran commented 1 year ago

+1 to this feature!

fczuardi commented 10 months ago

What would be the best return for the exec.command in this case? Log the error and return an empty string? Log the error and return the exit status as a string? Or return the same string that is being printed in the log.fatal (but without interrupting the main execution?

fczuardi commented 10 months ago

Out of curiosity, I went looking for the forks of this project and it seems that changing this is a popular motivator forforking.

Some replace this log.Fatal with a log.Print, like https://github.com/grafana/xk6-exec/compare/main...eznd-go:xk6-exec:main and and https://github.com/grafana/xk6-exec/compare/main...fcurella:xk6-exec:main while other even put it under an option for backwards compatibility: https://github.com/grafana/xk6-exec/commit/60c5a41c8c256d2505745a007d5941b964f6e6c7

It would be nice to have one of the patches landed on the main project. Is there any reason not to do it? What is missing? The pull requests?

filhodanuvem commented 10 months ago

In my opinion it is just about creating the pull request, time has passed and I didn't do it. Go for it @fczuardi if that unblocks you in any way. The best approach for me would be changing the method Command to return also an error, the caller would be responsible to panic it or not but I understand that this might generates a big PR.

https://github.com/grafana/xk6-exec/blob/997ee1bd7ecd0be67bdb8f803c9683f465194f76/exec.go#L49-L58

romaia commented 5 months ago

So, this was bothering me as well, so I created PR #20 that allows to specify an option fatalError: false to disable the current behavior. When ommited, it will keep failing for backwards compatibility.

I also took the oportunity to add support for an env option that will set environment variables.