Putting --debug in the "wrong position" causes Gitlab CI Multi Runner to crash.

Summary

If you run the Gitlab CI Multi Runner 1.7.1 with --debug as the FIRST parameter, there's no problem, but if you make it the last one, for example:

C:\glrunner\gitlab-ci-multi-runner-windows-amd64.exe run --working-directory C:\glrunner --config C:\glrunner\config.toml --debug

You get a crash.

Steps to reproduce

Run C:\glrunner\gitlab-ci-multi-runner-windows-amd64.exe run --working-directory C:\glrunner --config C:\glrunner\config.toml --debug

Actual behavior

Panic.

Expected behavior

Don't Panic.

Should tell me that `--debug` is not valid as a flag for the run verb. 

My reaction should be to try `C:\glrunner\gitlab-ci-multi-runner-windows-amd64.exe run --working-directory C:\glrunner --config C:\glrunner\config.toml -- --debug` and then achieve :sweat_smile: desired result.

## Relevant logs and/or screenshots

Panic traceback:

```
[37;1mRuntime platform                                  [0;m  [37;1march[0;m=amd64 [37;1mos[0;m=windows [37;1mrevision[0;m=f896af7 [37;1mversion[0;m=1.7.1
panic: reflect: call of reflect.Value.Type on zero Value [recovered]
	panic: reflect: call of reflect.Value.Type on zero Value

goroutine 1 [running]:
panic(0x1412020, 0xc042313180)
	/usr/local/go/src/runtime/panic.go:500 +0x1af
main.main.func1()
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/main.go:32 +0x86
panic(0x1412020, 0xc042313180)
	/usr/local/go/src/runtime/panic.go:458 +0x251
reflect.Value.Type(0x0, 0x0, 0x0, 0xc042354040, 0xc0420b7bf0)
	/usr/local/go/src/reflect/value.go:1670 +0x22b
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers.convertMarshal(0x0, 0x0, 0x0, 0x1, 0xc0424992a0, 0xc042304540, 0xc0424992e0, 0x52363b)
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers/convert.go:47 +0x4a
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers.convertToString(0x0, 0x0, 0x0, 0x0, 0x0, 0x14f5dc0, 0xc042236a80, 0x444578, 0x2dc62e6d1d9)
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers/convert.go:58 +0x64
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers.StructFieldValue.String(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers/struct_field.go:32 +0x82
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/gitlab.com/ayufan/golang-cli-helpers.(*StructFieldValue).String(0xc042236a80, 0x14f5dc0, 0xc042236a80)
	<autogenerated>:5 +0x81
flag.isZeroValue(0xc042301540, 0xc042312f40, 0x17, 0x1403d1e)
	/usr/local/go/src/flag/flag.go:393 +0x134
flag.(*FlagSet).PrintDefaults.func1(0xc042301540)
	/usr/local/go/src/flag/flag.go:467 +0x1dc
flag.(*FlagSet).VisitAll(0xc042041320, 0xc042499588)
	/usr/local/go/src/flag/flag.go:323 +0x6e
flag.(*FlagSet).PrintDefaults(0xc042041320)
	/usr/local/go/src/flag/flag.go:476 +0x4d
flag.defaultUsage(0xc042041320)
	/usr/local/go/src/flag/flag.go:509 +0x9d
flag.(*FlagSet).usage(0xc042041320)
	/usr/local/go/src/flag/flag.go:825 +0x5c
flag.(*FlagSet).failf(0xc042041320, 0x15fff29, 0x22, 0xc042499790, 0x1, 0x1, 0xc042499738, 0x47ebf5)
	/usr/local/go/src/flag/flag.go:814 +0x11d
flag.(*FlagSet).parseOne(0xc042041320, 0x1, 0x0, 0x0)
	/usr/local/go/src/flag/flag.go:873 +0xadf
flag.(*FlagSet).Parse(0xc042041320, 0xc042072020, 0x5, 0x5, 0x2fcbaf8, 0x0)
	/usr/local/go/src/flag/flag.go:915 +0x67
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/github.com/codegangsta/cli.Command.Run(0x15db5ec, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x15f5c13, 0x18, 0x0, ...)
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/github.com/codegangsta/cli/command.go:98 +0xc3a
gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/github.com/codegangsta/cli.(*App).Run(0xc04203cb00, 0xc042072000, 0x7, 0x7, 0x0, 0x0)
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/vendor/github.com/codegangsta/cli/app.go:159 +0x70c
main.main()
	/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/main.go:58 +0x256
```
## Environment description

Windows AMD 64 runner on private Windows 10 machine.

### Used GitLab Runner version
```
Version:      1.7.1
Git revision: f896af7
Git branch:   1-7-stable
GO version:   go1.7.3
Built:        Tue, 25 Oct 2016 16:57:56 +0000
OS/Arch:      windows/amd64
```

Added ~1009246 label

Added ~58730 label

This issue is generated by how github.com/codegangsta/cli works and should be probably fixed at upstream.

The problem here is that run is a subcommand and the --debug is the general use flag. We can't mess flags of general command (which is the gitlab-ci-multi-runner binary) with flags used by subcommands - that's how codegangsta/cli is working. You call your binary, then put all needed flags of the binary, then use the subcommand (if any are available, because you can write a software which doesn't use subcommands; our Runner however is using them), and then put all flags and/or arguments of the command (and then the same for the subcommand of subcommand and so on).

I'm afraid that allowing --debug and other general level flags to be used on any place in the command line call will add unnecessary complexity to the code.

The second problem is that there is a go panic, when a unknown flag is passed to subcommand. And there is now panic if anyone pass unknown flag on the general level - then the usage message is print. This is in my opinion a bug in github.com/codegangsta/cli and should be reported and fixed there. And after this we should upgrade vendored version of it.

So there's no optional error handling "callbacks" in the upstream library that are not set, leading it to panic because the less drastic handlers weren't provided? Even having it not panic, but say "--debug should be before the verb run" instead would be a much better user interface. The upstream library says that CLI programs should be fun. End users will perceive Panic as a bug in the CI Runner, so supporting arbitrary --debug position is not important, if you can just say "put that before the verb" and quietly stop.

@warren.postma

So there's no optional error handling "callbacks" in the upstream library that are not set, leading it to panic because the less drastic handlers weren't provided?

As far as I've checked - no. But I could miss something. If there is something like that, then yes, we should try to use it.

As for the rest of your comment - I agree that current situation is bad. User should not see panic if he only put an option in invalid place of command call. But instead of catching the panic and put a text "put that before the other" I would prefer to check why upstream library isn't showing usage message for subcommands by default. And then - if this is a bug in upstream library, then fix it there and upgrade our vendored version, and if this is our bad usage of the library - then fix the usage.

github.com/codegansta/cli should now be gopkg.in/urfave/cli.v1 (v2 is still unstable). We're using v1.8.0-4-g142e6cd - the latest is v1.19.1 and there are a number of closed issues relating to panics, although I haven't checked them to see if they're relevant.

Definitely worth trying an upgrade to begin with. I've labelled it ~up-for-grabs.

I will try that myself, as I'm also debugging a crash/hangup on a runner on Windows 10 tomorrow.

Mentioned in issue #1927 (closed)

I will take this one if you like. Here's what I've done so far:

1. I locally de-vendored the rather ancient codegangsta/cli
2. I search/replaced and go-get'd latest urfave/cli (but could go back to v1 if you want)
3. I tested like this, with result that I believe the issue is a bad/stale/vendored-crusty-codegangsta/cli:

gitlab-ci-multi-runner-windows-amd64.exe run --working-directory c:\glrunner -- --debug
 (worked!)
gitlab-ci-multi-runner-windows-amd64.exe --debug run --working-directory c:\glrunner 
 (worked!)
gitlab-ci-multi-runner-windows-amd64.exe run --working-directory c:\glrunner  --debug
 (worked/gave a message that --debug is not an option to the RUN command and did NOT panic.)

If I take this one should I be re-vendoring the v1 or just depend on go-get of the stable v1?

The urfave\cli parser's output in the case where --debug is given but does not apply to current verb, that replaces the panic is:

Incorrect Usage: flag provided but not defined: -debug
<dumps usage/help>
FATAL: ....

And no Panic.

@warren.postma Do you have a MR incoming? Else I would like to upgrade the dependency.

I have no MR incoming. Please do upgrade it.

Started, but need upstream merge first.

https://gitlab.com/ayufan/golang-cli-helpers/merge_requests/2

/cc @ayufan

mentioned in issue #2358 (closed)

mentioned in issue #2350 (closed)

changed milestone to %Backlog

mentioned in issue #2353

mentioned in issue #2348 (closed)

mentioned in issue #2425 (closed)

added Accepting Merge Requests label

@zj Upstream on this was merged 3 months ago, is this a go now?

@simon-buchan-nz Ill try to get to this in this release!

assigned to @zj

mentioned in merge request !656 (merged)

Updating the dependency did not fix the problem, so for now I'll remove myself as an assignee.

removed assignee