deis / workflow-cli

The CLI for Deis Workflow
http://deis.com
MIT License
31 stars 43 forks source link

fix(deis.go): do not add top level command to cmdArgs #255

Closed smt116 closed 7 years ago

smt116 commented 7 years ago

Ported from V1: https://github.com/deis/deis/pull/5099.


Background

We have a private PaaS with multiple Deis clusters (staging + two productions at the moment). We have also a tool that allows us to manage all clusters from a single web panel. It also connects other APIs such as servers provider and DNS provider. As the result we can perform actions that touches many parts of the system (for example create DNS records for all Deis workers and make CNAME to use with DEISCTL_TUNNEL or create a database and assign DATABASE_URL for a given application). We have also created plugins to extend Deis CLI. For example there is a support for database (+backups) management and easy SSH access for our developers.

Problem

We have deis backups:[create|show|restore|list] commands and we want to show usage help if user tries deis backups. There is a problem in such case because Deis CLI invokes deis-backups backups. Without additional support on the plugin side, it will throw an invalid command error.

Solution

We can solve it on the plugin side to handle unexpected arguments, but it feels not okay. Instead, Deis CLI should skip top level command (backups in this case) in cmdArgv. It already does that when used as deis backups:list or even deis backups:.

Code snippets

The output contains output using the following patch:

diff --git a/deis.go b/deis.go
index 6096618..85e574e 100644
--- a/deis.go
+++ b/deis.go
@@ -165,6 +165,12 @@ Use 'git push deis master' to deploy to an application.

                cmdArgv := prepareCmdArgs(command, argv)

+               fmt.Fprintf(wErr, "command: %s\n", command)
+               fmt.Fprintf(wErr, "argv: %s\n", argv)
+               fmt.Fprintf(wErr, "binary: %v\n", binary)
+               fmt.Fprintf(wErr, "cmdArgv: %v\n", cmdArgv)
+               return 1
+
                err = syscall.Exec(binary, cmdArgv, env)
                if err != nil {
                        parser.PrintUsage(&cmdr)

After

workflow-cli λ ./_dist/deis backups
command: backups
argv: [backups]
binary: /Users/smefju/.rbenv/shims/deis-backups
cmdArgv: [deis-backups]

workflow-cli λ ./_dist/deis backups:list
command: backups
argv: [backups:list]
binary: /Users/smefju/.rbenv/shims/deis-backups
cmdArgv: [deis-backups list]

workflow-cli λ ./_dist/deis backups --info
command: backups
argv: [backups --info]
binary: /Users/smefju/.rbenv/shims/deis-backups
cmdArgv: [deis-backups --info]

workflow-cli λ ./_dist/deis backups:create --info
command: backups
argv: [backups:create --info]
binary: /Users/smefju/.rbenv/shims/deis-backups
cmdArgv: [deis-backups create --info]

@bacongobbler I have extracted code to the prepareCmdArgs function to make it testable. I can make it the same in V1 if approach is okay. There is one spec missing:

func TestNastedCommandArgsPreparing(t *testing.T) {
    t.Parallel()

    command := "ssh"
    argv := []string{"ssh:info"}
    expected := []string{"deis-ssh info"}
    actual := prepareCmdArgs(command, argv)

    // t.Errorf("Expected %v, Got %v", reflect.TypeOf(expected), reflect.TypeOf(actual))
    if !reflect.DeepEqual(expected, actual) {
        t.Errorf("Expected %v, Got %v", expected, actual)
    }
}

I have got the following error:

--- FAIL: TestNastedCommandArgsPreparing (0.00s)
    deis_test.go:88: Expected [deis-ssh info], Got [deis-ssh info]

Both objects are []string type. Not sure where the issue is.

deis-bot commented 7 years ago

@Joshua-Anderson, @bacongobbler and @ultimateboy are potential reviewers of this pull request based on my analysis of git blame information. Thanks @smt116!

bacongobbler commented 7 years ago

Thanks for the unit tests!

codecov-io commented 7 years ago

Current coverage is 72.29% (diff: 50.00%)

Merging #255 into master will increase coverage by 0.08%

@@             master       #255   diff @@
==========================================
  Files            57         57          
  Lines          3904       3905     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2819       2823     +4   
+ Misses          777        773     -4   
- Partials        308        309     +1   

Powered by Codecov. Last update a6cd7e6...1622cf6