gardenctl icon indicating copy to clipboard operation
gardenctl copied to clipboard

reduce usage of checkError to make it possible to write tests

Open andrei-panov opened this issue 4 years ago • 4 comments

We have a very generic function checkError in utils.go file. The fatal flaw of this function that if there a non-nil error then it call for log.Fatalf(err.Error()) and exit. It's mean that if this function is used somewhere then it makes it not possible to test the calling function because it will exit during the test execution. It also prevents us from doing some cleaning after the executions if we created some files or other stateful manipulations.

A generic approach would be to check for errors during the code flow and if there is an error then pass it up to the stack call. Somewhere at the very top of the call chain, we can execute this function and finally exit if there an error.

At the moment we call this function 394 times

andrei-panov avatar Nov 24 '20 14:11 andrei-panov

💯

petersutter avatar Nov 24 '20 14:11 petersutter

yes also observe this in https://github.com/gardener/gardenctl/pull/404#discussion_r514777299. the checkError not return and exit after invoked, haven't got an idea of how to improve it at that time.

tedteng avatar Nov 25 '20 00:11 tedteng

Also the os.Exit calls should be removed or at least reduced. See also this comment from @tedteng https://github.com/gardener/gardenctl/issues/328#issuecomment-734033925 Instead, like @andrei-panov wrote, the error should bubble up the call stack.

petersutter avatar Nov 26 '20 09:11 petersutter

Do we have any guidelines? Since it's not possible to fix all places in one shot at least we can collect and document some desired (soft, flexible...) directions of future development.

andrei-panov avatar Dec 01 '20 13:12 andrei-panov