feat/cli: migrate version subcommand to use urface/cli#1292
feat/cli: migrate version subcommand to use urface/cli#1292
Conversation
keegancsmith
left a comment
There was a problem hiding this comment.
I'm kinda confused how things worked before for subcommands and flags... but yeah if your testing works (especially trying out cli flags for the subcommand), then lets do it.
Is there a wayt o migrate future commands which don't break them up so much / adjust indentation so much. That would make the PRs much easier to validate.
| exitCode, err = runLegacy(cmd, flagSet) | ||
| } | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Shouldn't we now log.Println this since we always want to use the returned exitCode?
| "github.com/sourcegraph/sourcegraph/lib/errors" | ||
| ) | ||
|
|
||
| var MigratedCommands = map[string]*cli.Command{ |
There was a problem hiding this comment.
minor this doesn't need to be exported
| // runMigrated runs the command within urfave/cli framework | ||
| func runMigrated(flagSet *flag.FlagSet) (int, error) { | ||
| ctx := context.Background() | ||
| args := append([]string{"src"}, flagSet.Args()...) |
There was a problem hiding this comment.
I don't think this works since it won't include any flag arguments rights? This is just everything after command line flags?
I think the only reason it works right now is you have only miugrated version
| args := flagSet.Args()[1:] | ||
| if err := cmd.flagSet.Parse(args); err != nil { |
There was a problem hiding this comment.
I'm kinda confused how this all worked before. .Args is the non flag arguments, not the raw argv.
| fmt.Printf("Current version: %s\n", version.BuildTag) | ||
| if clientOnly != nil && *clientOnly { | ||
| return nil | ||
| var versionCommandv2 = clicompat.Wrap(&cli.Command{ |
There was a problem hiding this comment.
Mostly just to distinguish it from the normal version command but I realize now it's a misnomer, since there isn't a versionCommand 🤡
I'll update it!
| for _, cmd := range c { | ||
| if !cmd.matches(name) { | ||
| _, isMigratedCmd := MigratedCommands[name] | ||
| if !isMigratedCmd && !cmd.matches(name) { |
There was a problem hiding this comment.
this is kinda funky to read and will also break when one day c is empty.
you don't do that much inside of this for loop. Why don't you pull out a check for MigratedCommands before this for loop and if you have a match in it, just have a simpler bit of code to run that command (with readConfig/etc duplicated)
There was a problem hiding this comment.
Agreed! I'll move it
There was a problem hiding this comment.
Another reason to move it, I just realised it has this issue with how its currently implemented. Any subcommand can be a global migrated command:
❯ go run ./cmd/src auth version
Current version: dev
Recommended version: 7.0.3 or later
There was a problem hiding this comment.
Well now that is awkward 🤣 Good spot! Push the fix in a few
|
@burmudar I realised how it works now and came back to comment why. golangs flagset packages will treat all arguments after the first non flag as arguments. So |
This introduces a compatibility layer to start migrating src-cli to use a cli framework.
Some of the benefits we get by using a cli framework:
If a command is within the migrated set, we then defer to executing the command within urfave/cli with
runMigrated. If the command is not in the set we executing the legacy code withrunLegacy.Test plan
Tested locally