Validate script arguments (#43)

* Validate script arguments

If a user provides unknown arguments to a script this is silently
ignored.

This can lead to time consuming typos, e.g. 'versoin' instead of
'version'.

This change set ensures that no unknown arguments can be applied to a
script. It also detects all errors before stopping execution so unknown
arguments, missing required arguments and non-parsable arguments are
reported in one go.

$ shuttle --project examples/moon-base run required-arg b=1 d
shuttle failed
Arguments not valid:
 'd' not <argument>=<value>
 'a' not supplied but is required
 'b' unknown

Script 'required-arg' accepts the following arguments:
  a (required)

Lastly acceptable arguments are printed on any of the above errors to
help users detect their mistakes fast.

* Sort validation errors for consitent output

pkg/config/shuttleplan.go

@@ -6,6 +6,7 @@ import (
 	"path"
 	"path/filepath"
 	"regexp"
+	"strings"
 
 	"fmt"
 	"os"
@@ -29,6 +30,18 @@ type ShuttleScriptArgs struct {
 	Description string `yaml:"description"`
 }
 
+func (a ShuttleScriptArgs) String() string {
+	var s strings.Builder
+	s.WriteString(a.Name)
+	if a.Required {
+		s.WriteString(" (required)")
+	}
+	if len(a.Description) != 0 {
+		fmt.Fprintf(&s, "  %s", a.Description)
+	}
+	return s.String()
+}
+
 // ShuttleAction describes an action done by a shuttle script
 type ShuttleAction struct {
 	Shell      string `yaml:"shell"`

pkg/executors/executor.go

@@ -2,6 +2,7 @@ package executors
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/lunarway/shuttle/pkg/config"
@@ -26,22 +27,10 @@ type ActionExecutionContext struct {
 func Execute(p config.ShuttleProjectContext, command string, args []string) {
 	script, ok := p.Scripts[command]
 	if !ok {
-		p.UI.ExitWithErrorCode(2, "No script found called '%s'", command)
-	}
-	namedArgs := map[string]string{}
-	for _, arg := range args {
-		parts := strings.SplitN(arg, "=", 2)
-		if len(parts) < 2 {
-			p.UI.ExitWithError("Could not parse `shuttle run %s %s`, because '%s' was expected to be on the `<option>=<value>` form, but wasn't!.\nScript '%s' expects arguments:\n%v", command, strings.Join(args, " "), arg, command, script.Args)
-		}
-		namedArgs[parts[0]] = parts[1]
+		p.UI.ExitWithErrorCode(2, "Script '%s' not found", command)
 	}
 
-	for _, argSpec := range script.Args {
-		if _, ok := namedArgs[argSpec.Name]; argSpec.Required && !ok {
-			p.UI.ExitWithError("Required argument `%s` for script `%s` was not supplied!", argSpec.Name, command) // TODO: Add expected arguments
-		}
-	}
+	namedArgs := validateArguments(p, command, script.Args, args)
 
 	scriptContext := ScriptExecutionContext{
 		ScriptName: command,
@@ -60,6 +49,109 @@ func Execute(p config.ShuttleProjectContext, command string, args []string) {
 	}
 }
 
+// validateArguments parses and validates args against available arguments in
+// scriptArgs.
+//
+// All detectable constraints are checked before reporting to the UI.
+func validateArguments(p config.ShuttleProjectContext, command string, scriptArgs []config.ShuttleScriptArgs, args []string) map[string]string {
+	var validationErrors []validationError
+
+	namedArgs, parsingErrors := validateArgFormat(args)
+	validationErrors = append(validationErrors, parsingErrors...)
+	validationErrors = append(validationErrors, validateRequiredArgs(scriptArgs, namedArgs)...)
+	validationErrors = append(validationErrors, validateUnknownArgs(scriptArgs, namedArgs)...)
+	if len(validationErrors) != 0 {
+		sortValidationErrors(validationErrors)
+		var s strings.Builder
+		s.WriteString("Arguments not valid:\n")
+		for _, e := range validationErrors {
+			fmt.Fprintf(&s, " %s\n", e)
+		}
+		fmt.Fprintf(&s, "\n%s", expectedArgumentsHelp(command, scriptArgs))
+		p.UI.ExitWithError(s.String())
+	}
+	return namedArgs
+}
+
+type validationError struct {
+	arg string
+	err string
+}
+
+func (v validationError) String() string {
+	return fmt.Sprintf("'%s' %s", v.arg, v.err)
+}
+
+func validateArgFormat(args []string) (map[string]string, []validationError) {
+	var validationErrors []validationError
+	namedArgs := map[string]string{}
+	for _, arg := range args {
+		parts := strings.SplitN(arg, "=", 2)
+		if len(parts) < 2 {
+			validationErrors = append(validationErrors, validationError{
+				arg: arg,
+				err: "not <argument>=<value>",
+			})
+			continue
+		}
+		namedArgs[parts[0]] = parts[1]
+	}
+	return namedArgs, validationErrors
+}
+
+func validateRequiredArgs(scriptArgs []config.ShuttleScriptArgs, args map[string]string) []validationError {
+	var validationErrors []validationError
+	for _, argSpec := range scriptArgs {
+		if _, ok := args[argSpec.Name]; argSpec.Required && !ok {
+			validationErrors = append(validationErrors, validationError{
+				arg: argSpec.Name,
+				err: "not supplied but is required",
+			})
+		}
+	}
+	return validationErrors
+}
+
+func validateUnknownArgs(scriptArgs []config.ShuttleScriptArgs, args map[string]string) []validationError {
+	var validationErrors []validationError
+	for namedArg := range args {
+		found := false
+		for _, arg := range scriptArgs {
+			if arg.Name == namedArg {
+				found = true
+				break
+			}
+		}
+		if !found {
+			validationErrors = append(validationErrors, validationError{
+				arg: namedArg,
+				err: "unknown",
+			})
+		}
+	}
+	return validationErrors
+}
+
+func sortValidationErrors(errs []validationError) {
+	sort.Slice(errs, func(i, j int) bool {
+		return errs[i].arg < errs[j].arg
+	})
+}
+
+func expectedArgumentsHelp(command string, args []config.ShuttleScriptArgs) string {
+	var s strings.Builder
+	fmt.Fprintf(&s, "Script '%s' accepts ", command)
+	if len(args) == 0 {
+		s.WriteString("no arguments.")
+		return s.String()
+	}
+	s.WriteString("the following arguments:")
+	for _, a := range args {
+		fmt.Fprintf(&s, "\n  %s", a)
+	}
+	return s.String()
+}
+
 func executeAction(context ActionExecutionContext) {
 	for _, executor := range executors {
 		if executor.match(context.Action) {

pkg/executors/executor_test.go

@@ -0,0 +1,142 @@
+package executors
+
+import (
+	"testing"
+
+	"github.com/lunarway/shuttle/pkg/config"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateUnknownArgs(t *testing.T) {
+	tt := []struct {
+		name       string
+		scriptArgs []config.ShuttleScriptArgs
+		inputArgs  map[string]string
+		output     []validationError
+	}{
+		{
+			name:       "no input or script",
+			scriptArgs: nil,
+			inputArgs:  nil,
+			output:     nil,
+		},
+		{
+			name:       "single input without script",
+			scriptArgs: nil,
+			inputArgs: map[string]string{
+				"foo": "1",
+			},
+			output: []validationError{
+				{"foo", "unknown"},
+			},
+		},
+		{
+			name:       "multiple input without script",
+			scriptArgs: nil,
+			inputArgs: map[string]string{
+				"foo": "1",
+				"bar": "2",
+			},
+			output: []validationError{
+				{"bar", "unknown"},
+				{"foo", "unknown"},
+			},
+		},
+		{
+			name: "single input and script",
+			scriptArgs: []config.ShuttleScriptArgs{
+				{
+					Name: "foo",
+				},
+			},
+			inputArgs: map[string]string{
+				"foo": "1",
+			},
+			output: nil,
+		},
+		{
+			name: "multple input and script",
+			scriptArgs: []config.ShuttleScriptArgs{
+				{
+					Name: "foo",
+				},
+				{
+					Name: "bar",
+				},
+			},
+			inputArgs: map[string]string{
+				"bar": "2",
+				"foo": "1",
+			},
+			output: nil,
+		},
+		{
+			name: "multple input and script with one unknown",
+			scriptArgs: []config.ShuttleScriptArgs{
+				{
+					Name: "foo",
+				},
+				{
+					Name: "bar",
+				},
+			},
+			inputArgs: map[string]string{
+				"foo": "1",
+				"bar": "2",
+				"baz": "3",
+			},
+			output: []validationError{
+				{"baz", "unknown"},
+			},
+		},
+	}
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			output := validateUnknownArgs(tc.scriptArgs, tc.inputArgs)
+			// sort as the order is not guarenteed by validateUnknownArgs
+			sortValidationErrors(output)
+			assert.Equal(t, tc.output, output, "output not as expected")
+		})
+	}
+}
+
+func TestSortValidationErrors(t *testing.T) {
+	tt := []struct {
+		name   string
+		input  []validationError
+		output []validationError
+	}{
+		{
+			name: "sorted",
+			input: []validationError{
+				{"bar", ""},
+				{"baz", ""},
+				{"foo", ""},
+			},
+			output: []validationError{
+				{"bar", ""},
+				{"baz", ""},
+				{"foo", ""},
+			},
+		},
+		{
+			name: "not sorted",
+			input: []validationError{
+				{"baz", ""},
+				{"foo", ""},
+				{"bar", ""},
+			},
+			output: []validationError{
+				{"bar", ""},
+				{"baz", ""},
+				{"foo", ""},
+			},
+		},
+	}
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			sortValidationErrors(tc.input)
+			assert.Equal(t, tc.output, tc.input, "output not as expected")
+		})
+	}
+}

tests.sh

@@ -159,6 +159,11 @@ test_run_shell_error_outputs_missing_arg() {
   fi
 }
 
+test_run_shell_error_outputs_unknown_argument() {
+  assertErrorCode 1 -p examples/moon-base run required-arg a=baz foo=bar
+  assertContains "'foo' unknown" "$result"
+}
+
 test_template_local_path() {
   assertErrorCode 0 -p examples/moon-base template ../custom-template.tmpl -o Dockerfile-custom
 }