tiltfile: StringOrStringList: require Union[string,Union[List,Tuple]]…

… instead of Union[string,Iterable] (#3572)

internal/tiltfile/docker.go

@@ -93,11 +93,9 @@ func (s *tiltfileState) dockerBuild(thread *starlark.Thread, fn *starlark.Builti
 		liveUpdateVal,
 		ignoreVal,
 		onlyVal,
-		sshVal,
-		secretVal,
 		networkVal,
-		entrypoint,
-		extraTagsVal starlark.Value
+		entrypoint starlark.Value
+	var ssh, secret, extraTags value.StringOrStringList
 	var matchInEnvVars bool
 	var containerArgsVal starlark.Sequence
 	if err := s.unpackArgs(fn.Name(), args, kwargs,
@@ -114,10 +112,10 @@ func (s *tiltfileState) dockerBuild(thread *starlark.Thread, fn *starlark.Builti
 		"entrypoint?", &entrypoint,
 		"container_args?", &containerArgsVal,
 		"target?", &targetStage,
-		"ssh?", &sshVal,
-		"secret?", &secretVal,
+		"ssh?", &ssh,
+		"secret?", &secret,
 		"network?", &networkVal,
-		"extra_tag?", &extraTagsVal,
+		"extra_tag?", &extraTags,
 	); err != nil {
 		return nil, err
 	}
@@ -192,18 +190,9 @@ func (s *tiltfileState) dockerBuild(thread *starlark.Thread, fn *starlark.Builti
 		return nil, err
 	}
 
-	ssh, ok := value.AsStringOrStringList(sshVal)
-	if !ok {
-		return nil, fmt.Errorf("Argument 'ssh' must be string or list of strings. Actual: %T", sshVal)
-	}
-
-	secret, ok := value.AsStringOrStringList(secretVal)
-	if !ok {
-		return nil, fmt.Errorf("Argument 'secret' must be string or list of strings. Actual: %T", secretVal)
-	}
-
 	network := ""
 	if networkVal != nil {
+		var ok bool
 		network, ok = value.AsString(networkVal)
 		if !ok {
 			return nil, fmt.Errorf("Argument 'network' must be string. Actual: %T", networkVal)
@@ -224,12 +213,7 @@ func (s *tiltfileState) dockerBuild(thread *starlark.Thread, fn *starlark.Builti
 		containerArgs = model.OverrideArgs{ShouldOverride: true, Args: args}
 	}
 
-	extraTags, ok := value.AsStringOrStringList(extraTagsVal)
-	if !ok {
-		return nil, fmt.Errorf("Argument 'extra_tag' must be string or list of strings. Actual: %T", extraTagsVal)
-	}
-
-	for _, extraTag := range extraTags {
+	for _, extraTag := range extraTags.Values {
 		_, err := container.ParseNamed(extraTag)
 		if err != nil {
 			return nil, fmt.Errorf("Argument extra_tag=%q not a valid image reference: %v", extraTag, err)
@@ -245,15 +229,15 @@ func (s *tiltfileState) dockerBuild(thread *starlark.Thread, fn *starlark.Builti
 		dbBuildArgs:      sba,
 		liveUpdate:       liveUpdate,
 		matchInEnvVars:   matchInEnvVars,
-		sshSpecs:         ssh,
-		secretSpecs:      secret,
+		sshSpecs:         ssh.Values,
+		secretSpecs:      secret.Values,
 		ignores:          ignores,
 		onlys:            onlys,
 		entrypoint:       entrypointCmd,
 		containerArgs:    containerArgs,
 		targetStage:      targetStage,
 		network:          network,
-		extraTags:        extraTags,
+		extraTags:        extraTags.Values,
 	}
 	err = s.buildIndex.addImage(r)
 	if err != nil {

internal/tiltfile/files.go

@@ -121,14 +121,14 @@ func (s *tiltfileState) helm(thread *starlark.Thread, fn *starlark.Builtin, args
 	var path starlark.Value
 	var name string
 	var namespace string
-	var valueFilesV starlark.Value
-	var setV starlark.Value
+	var valueFiles value.StringOrStringList
+	var set value.StringOrStringList
 	err := s.unpackArgs(fn.Name(), args, kwargs,
 		"paths", &path,
 		"name?", &name,
 		"namespace?", &namespace,
-		"values?", &valueFilesV,
-		"set?", &setV)
+		"values?", &valueFiles,
+		"set?", &set)
 	if err != nil {
 		return nil, err
 	}
@@ -138,17 +138,6 @@ func (s *tiltfileState) helm(thread *starlark.Thread, fn *starlark.Builtin, args
 		return nil, fmt.Errorf("Argument 0 (paths): %v", err)
 	}
 
-	valueFiles, ok := value.AsStringOrStringList(valueFilesV)
-	if !ok {
-		return nil, fmt.Errorf("Argument 'values' must be string or list of strings. Actual: %T",
-			valueFilesV)
-	}
-
-	set, ok := value.AsStringOrStringList(setV)
-	if !ok {
-		return nil, fmt.Errorf("Argument 'set' must be string or list of strings. Actual: %T", setV)
-	}
-
 	info, err := os.Stat(localPath)
 	if err != nil {
 		if os.IsNotExist(err) {
@@ -194,14 +183,14 @@ func (s *tiltfileState) helm(thread *starlark.Thread, fn *starlark.Builtin, args
 	if namespace != "" {
 		cmd = append(cmd, "--namespace", namespace)
 	}
-	for _, valueFile := range valueFiles {
+	for _, valueFile := range valueFiles.Values {
 		cmd = append(cmd, "--values", valueFile)
 		err := tiltfile_io.RecordReadPath(thread, tiltfile_io.WatchFileOnly, starkit.AbsPath(thread, valueFile))
 		if err != nil {
 			return nil, err
 		}
 	}
-	for _, setArg := range set {
+	for _, setArg := range set.Values {
 		cmd = append(cmd, "--set", setArg)
 	}
 

internal/tiltfile/helm_test.go

@@ -41,6 +41,20 @@ k8s_yaml(yml)
 	assert.Contains(t, yaml, "servicePort: 1234")
 }
 
+func TestHelmSetArgsMap(t *testing.T) {
+	f := newFixture(t)
+	defer f.TearDown()
+
+	f.setupHelm()
+
+	f.file("Tiltfile", `
+yml = helm('./helm', name='rose-quartz', namespace='garnet', set={'a': 'b'})
+k8s_yaml(yml)
+`)
+
+	f.loadErrString("helm: for parameter \"set\"", "string", "List", "type dict")
+}
+
 const exampleHelmV2VersionOutput = `Client: v2.12.3geecf22f`
 const exampleHelmV3VersionOutput = `v3.0.0`
 

internal/tiltfile/value/string.go

@@ -1,6 +1,10 @@
 package value
 
-import "go.starlark.net/starlark"
+import (
+	"fmt"
+
+	"go.starlark.net/starlark"
+)
 
 type ImplicitStringer interface {
 	ImplicitString() string
@@ -15,33 +19,45 @@ func AsString(x starlark.Value) (string, bool) {
 	return starlark.AsString(x)
 }
 
+type StringOrStringList struct {
+	Values []string
+}
+
+var _ starlark.Unpacker = &StringOrStringList{}
+
 // Unpack an argument that can either be expressed as
 // a string or as a list of strings.
-func AsStringOrStringList(x starlark.Value) ([]string, bool) {
-	if x == nil {
-		return []string{}, true
+func (s *StringOrStringList) Unpack(v starlark.Value) error {
+	s.Values = nil
+	if v == nil {
+		return nil
 	}
 
-	s, ok := AsString(x)
+	vs, ok := AsString(v)
 	if ok {
-		return []string{s}, true
+		s.Values = []string{vs}
+		return nil
 	}
 
-	iterable, ok := x.(starlark.Iterable)
-	if ok {
-		result := []string{}
-		iter := iterable.Iterate()
-		defer iter.Done()
-		var item starlark.Value
-		for iter.Next(&item) {
-			s, ok := AsString(item)
-			if !ok {
-				return nil, false
-			}
-			result = append(result, s)
+	var iter starlark.Iterator
+	switch x := v.(type) {
+	case *starlark.List:
+		iter = x.Iterate()
+	case starlark.Tuple:
+		iter = x.Iterate()
+	default:
+		return fmt.Errorf("value should be a string or List or Tuple of strings, but is of type %s", v.Type())
+	}
+
+	defer iter.Done()
+	var item starlark.Value
+	for iter.Next(&item) {
+		sv, ok := AsString(item)
+		if !ok {
+			return fmt.Errorf("list should contain only strings, but element %q was of type %s", item.String(), item.Type())
 		}
-		return result, true
+		s.Values = append(s.Values, sv)
 	}
 
-	return nil, false
+	return nil
 }

internal/tiltfile/value/string_test.go

@@ -0,0 +1,56 @@
+package value
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.starlark.net/starlark"
+)
+
+func TestAsStringOrStringList_String(t *testing.T) {
+	var v StringOrStringList
+	err := v.Unpack(starlark.String("foo"))
+
+	require.NoError(t, err)
+	require.Equal(t, []string{"foo"}, v.Values)
+}
+
+func TestAsStringOrStringList_ListOfStrings(t *testing.T) {
+	var v StringOrStringList
+	err := v.Unpack(starlark.NewList([]starlark.Value{
+		starlark.String("foo"),
+		starlark.String("bar"),
+		starlark.String("baz"),
+	}))
+
+	require.NoError(t, err)
+	require.Equal(t, []string{"foo", "bar", "baz"}, v.Values)
+}
+
+func TestAsStringOrStringList_NonStringOrList(t *testing.T) {
+	var v StringOrStringList
+	err := v.Unpack(starlark.Bool(true))
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "value should be a string or List or Tuple of strings, but is of type bool")
+}
+
+func TestAsStringOrStringList_ListWithNonStringElement(t *testing.T) {
+	var v StringOrStringList
+	err := v.Unpack(starlark.NewList([]starlark.Value{starlark.String("foo"), starlark.Bool(true)}))
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "list should contain only strings, but element \"True\" was of type bool")
+}
+
+// https://github.com/tilt-dev/tilt/issues/3570
+func TestAsStringOrStringList_Map(t *testing.T) {
+	m := starlark.NewDict(2)
+	err := m.SetKey(starlark.String("foo"), starlark.String("1"))
+	require.NoError(t, err)
+	err = m.SetKey(starlark.String("bar"), starlark.String("2"))
+	require.NoError(t, err)
+
+	var v StringOrStringList
+	err = v.Unpack(m)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "value should be a string or List or Tuple of strings, but is of type dict")
+}