Initial wiring #3
Conversation
|
OK should be good now. This adds the wiring with the |
cmd/runner.go
Outdated
| stderr io.Writer | ||
| } | ||
|
|
||
| func (r *runner) Run(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
I think we can have some RunE or RunError interface to do the logging and process termination in fewer places.
There was a problem hiding this comment.
That makes sense but where should it live? microkit?
There was a problem hiding this comment.
So when this is the method which is called by cobra then it is done there. Good question what happens on error then, but there is something like RunE and I thought we should rather use this and do normal error handling here.
There was a problem hiding this comment.
Got it. Yes using RunE is nice and the error is passed to Execute when it's called. I renamed Run to RunWithError to align with mainWithError.
@xh3b4sd PTAL
There was a problem hiding this comment.
I think this code is copied from devctl and I did it there for purpose. I think it's better when we handle all errors ourselves with this code
https://github.com/giantswarm/devctl/blob/master/cmd/runner.go#L30-L34
Otherwise there are ugly repeated error messages and usage messages in random places.
There was a problem hiding this comment.
Did you try using RunE? I did a basic test and the error was propagated to the Execute call.
Yes this is all based on what you created in devctl. I'd like both projects to be aligned. So if the existing approach is better I'm fine with using it.
There was a problem hiding this comment.
I have a blurry memories but something was bad.
How about stack trace? Was it printed with RunE?
There was a problem hiding this comment.
Ack. But yes the stack trace is preserved.
go run main.go version
Git Commit: n/a
Go Version: go1.11.2
OS / Arch: darwin / amd64
Source: https://github.com/giantswarm/e2ectl
Error: my test error: invalid config error
Usage:
e2ectl version [flags]
Flags:
-h, --help help for version
{"caller":"e2ectl/main.go:55","level":"error","message":"failed to execute root command","stack":"[{/Users/ross/Documents/giantswarm/src/e2ectl/cmd/version/runner.go:34: } {/Users/ross/Documents/giantswarm/src/e2ectl/cmd/version/runner.go:48: } {/Users/ross/Documents/giantswarm/src/e2ectl/cmd/version/runner.go:55: my test error} {invalid config error}]","time":"2019-04-09T13:53:07.853366+00:00"}
exit status 1
There was a problem hiding this comment.
Okay now I remember. The usage was an issue. I really dislike it. Let's say e2e-harness fails to setup a cluster and prints an error preceded with usage. That doesn't make sense because command was executed correctly. It just failed at runtime.
| source = "https://github.com/giantswarm/e2ectl" | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
mainError would also be good here.
| err = rootCommand.Execute() | ||
| if err != nil { | ||
| logger.LogCtx(ctx, "level", "error", "message", "failed to execute root command", "stack", fmt.Sprintf("%#v", err)) | ||
| os.Exit(1) |
There was a problem hiding this comment.
We can return an error here now.
There was a problem hiding this comment.
The problem is logging.
If I return an error we will panic with it. I'd rather use the logger unless we can't create it.
|
Going with this. I think the main.go is good enough for now. |
|
Deploy step fails because Circle config uses a docker build and not machine so there is no jq :( I'll sort that later. |
Adds initial wiring based on what we do in devctl.