From 3fdd3efd0d0a8202d6a9881c29ccf074cc766628 Mon Sep 17 00:00:00 2001 From: Federico Gimenez Date: Sat, 16 Dec 2017 21:39:50 +0100 Subject: [PATCH 1/2] fix(helm): do not ignore plugin arguments with ignoreFlag=true Makes it possible to not ignore the arguments given in a plugin call with ignoreFlag=true until the first flag is found, from there the rest is removed. Closes: #3218 --- docs/plugins.md | 4 ++- pkg/plugin/plugin.go | 13 ++++++-- pkg/plugin/plugin_test.go | 62 ++++++++++++++++++++++----------------- 3 files changed, 49 insertions(+), 30 deletions(-) diff --git a/docs/plugins.md b/docs/plugins.md index c14063cf8..cb383822b 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -90,7 +90,9 @@ Restrictions on `name`: The `ignoreFlags` switch tells Helm to _not_ pass flags to the plugin. So if a plugin is called with `helm myplugin --foo` and `ignoreFlags: true`, then `--foo` -is silently discarded. +is silently discarded. The arguments provided until the first tag are kept, so +`helm myplugin myarg --foo` and `ignoreFlags: true` will be equivalent to `helm +myplugin myarg`. The `useTunnel` switch indicates that the plugin needs a tunnel to Tiller. This should be set to `true` _anytime a plugin talks to Tiller_. It will cause Helm diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go index b3458c2d8..bfd102212 100644 --- a/pkg/plugin/plugin.go +++ b/pkg/plugin/plugin.go @@ -105,9 +105,18 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string) { if len(parts) > 1 { baseArgs = parts[1:] } - if !p.Metadata.IgnoreFlags { - baseArgs = append(baseArgs, extraArgs...) + extraIndex := len(extraArgs) + // determine the index of the first flag if we need to ignore them + if p.Metadata.IgnoreFlags { + for i, arg := range extraArgs { + if strings.HasPrefix(arg, "--") { + extraIndex = i + break + } + } } + baseArgs = append(baseArgs, extraArgs[0:extraIndex]...) + return main, baseArgs } diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go index 5ddbf15f3..e005ed0b1 100644 --- a/pkg/plugin/plugin_test.go +++ b/pkg/plugin/plugin_test.go @@ -28,37 +28,45 @@ func TestPrepareCommand(t *testing.T) { Command: "echo -n foo", }, } - argv := []string{"--debug", "--foo", "bar"} - - cmd, args := p.PrepareCommand(argv) - if cmd != "echo" { - t.Errorf("Expected echo, got %q", cmd) - } - - if l := len(args); l != 5 { - t.Errorf("expected 5 args, got %d", l) + testCases := []struct { + argv []string + expect []string + ignoreFlags bool + }{ + { + argv: []string{"--debug", "--foo", "bar"}, + expect: []string{"-n", "foo", "--debug", "--foo", "bar"}, + }, + { + argv: []string{}, + expect: []string{"-n", "foo"}, + }, + { + argv: []string{"--debug", "--foo", "bar"}, + expect: []string{"-n", "foo"}, + ignoreFlags: true, + }, + { + argv: []string{"arg1", "arg2", "--debug", "--foo", "bar", "arg3"}, + expect: []string{"-n", "foo", "arg1", "arg2"}, + ignoreFlags: true, + }, } + for _, tc := range testCases { + p.Metadata.IgnoreFlags = tc.ignoreFlags + cmd, args := p.PrepareCommand(tc.argv) + if cmd != "echo" { + t.Errorf("Expected echo, got %q", cmd) + } - expect := []string{"-n", "foo", "--debug", "--foo", "bar"} - for i := 0; i < len(args); i++ { - if expect[i] != args[i] { - t.Errorf("Expected arg=%q, got %q", expect[i], args[i]) + if l, le := len(args), len(tc.expect); l != le { + t.Errorf("expected %d args, got %d", le, l) } - } - // Test with IgnoreFlags. This should omit --debug, --foo, bar - p.Metadata.IgnoreFlags = true - cmd, args = p.PrepareCommand(argv) - if cmd != "echo" { - t.Errorf("Expected echo, got %q", cmd) - } - if l := len(args); l != 2 { - t.Errorf("expected 2 args, got %d", l) - } - expect = []string{"-n", "foo"} - for i := 0; i < len(args); i++ { - if expect[i] != args[i] { - t.Errorf("Expected arg=%q, got %q", expect[i], args[i]) + for i := 0; i < len(args); i++ { + if tc.expect[i] != args[i] { + t.Errorf("Expected arg=%q, got %q", tc.expect[i], args[i]) + } } } } From 5465f6b059fb787b1abe6982378085d51d2808c1 Mon Sep 17 00:00:00 2001 From: Federico Gimenez Date: Sun, 17 Dec 2017 08:56:54 +0100 Subject: [PATCH 2/2] short flags --- pkg/plugin/plugin.go | 2 +- pkg/plugin/plugin_test.go | 61 +++++++++++++++++++++++++++------------ 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go index bfd102212..b67c39928 100644 --- a/pkg/plugin/plugin.go +++ b/pkg/plugin/plugin.go @@ -109,7 +109,7 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string) { // determine the index of the first flag if we need to ignore them if p.Metadata.IgnoreFlags { for i, arg := range extraArgs { - if strings.HasPrefix(arg, "--") { + if strings.HasPrefix(arg, "-") { extraIndex = i break } diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go index e005ed0b1..cbebf5dd9 100644 --- a/pkg/plugin/plugin_test.go +++ b/pkg/plugin/plugin_test.go @@ -29,45 +29,70 @@ func TestPrepareCommand(t *testing.T) { }, } testCases := []struct { + description string argv []string expect []string ignoreFlags bool }{ { - argv: []string{"--debug", "--foo", "bar"}, - expect: []string{"-n", "foo", "--debug", "--foo", "bar"}, + description: "no extra args", + argv: []string{}, + expect: []string{"-n", "foo"}, }, { - argv: []string{}, - expect: []string{"-n", "foo"}, + description: "extra args, all long flags, keep flags", + argv: []string{"--debug", "--foo", "bar"}, + expect: []string{"-n", "foo", "--debug", "--foo", "bar"}, }, { + description: "extra args, all long flags, ignore flags", argv: []string{"--debug", "--foo", "bar"}, expect: []string{"-n", "foo"}, ignoreFlags: true, }, { + description: "extra args, arguments, long flags, ignore flags", argv: []string{"arg1", "arg2", "--debug", "--foo", "bar", "arg3"}, expect: []string{"-n", "foo", "arg1", "arg2"}, ignoreFlags: true, }, + { + description: "extra args, short (first) and long flags, ignore flags", + argv: []string{"-s", "--debug", "--foo", "bar", "arg3"}, + expect: []string{"-n", "foo"}, + ignoreFlags: true, + }, + { + description: "extra args, arguments, short (first) and long flags, ignore flags", + argv: []string{"arg1", "arg2", "-d", "--foo", "bar", "arg3"}, + expect: []string{"-n", "foo", "arg1", "arg2"}, + ignoreFlags: true, + }, + { + description: "extra args, long (first) and short flags, ignore flags", + argv: []string{"--debug", "-s", "--foo", "bar", "arg3"}, + expect: []string{"-n", "foo"}, + ignoreFlags: true, + }, } for _, tc := range testCases { - p.Metadata.IgnoreFlags = tc.ignoreFlags - cmd, args := p.PrepareCommand(tc.argv) - if cmd != "echo" { - t.Errorf("Expected echo, got %q", cmd) - } - - if l, le := len(args), len(tc.expect); l != le { - t.Errorf("expected %d args, got %d", le, l) - } - - for i := 0; i < len(args); i++ { - if tc.expect[i] != args[i] { - t.Errorf("Expected arg=%q, got %q", tc.expect[i], args[i]) + t.Run(tc.description, func(t *testing.T) { + p.Metadata.IgnoreFlags = tc.ignoreFlags + cmd, args := p.PrepareCommand(tc.argv) + if cmd != "echo" { + t.Errorf("Expected echo, got %q", cmd) + } + + if l, le := len(args), len(tc.expect); l != le { + t.Errorf("expected %d args, got %d", le, l) + } + + for i := 0; i < len(args); i++ { + if tc.expect[i] != args[i] { + t.Errorf("Expected arg=%q, got %q", tc.expect[i], args[i]) + } } - } + }) } }