tiltfile: properly model filewatches shared across multiple images (#…

…5648)

fixes #5621

internal/controllers/apis/configmap/disable.go

@@ -21,33 +21,51 @@ func DisableStatus(getCM func(name string) (v1alpha1.ConfigMap, error), disableS
 		return v1alpha1.DisableStateEnabled, "object does not specify a DisableSource", nil
 	}
 
-	if disableSource.ConfigMap == nil {
-		return v1alpha1.DisableStateError, "DisableSource specifies no ConfigMap", nil
+	if disableSource.ConfigMap != nil {
+		return cmDisableState(getCM, *disableSource.ConfigMap)
 	}
 
-	cm, err := getCM(disableSource.ConfigMap.Name)
+	if len(disableSource.EveryConfigMap) > 0 {
+		for _, cm := range disableSource.EveryConfigMap {
+			state, reason, err := cmDisableState(getCM, cm)
+			if state != v1alpha1.DisableStateDisabled {
+				return state, reason, err
+			}
+		}
+		return v1alpha1.DisableStateDisabled, "Every ConfigMap disabled", nil
+	}
+
+	return v1alpha1.DisableStateError, "DisableSource specifies no valid sources", nil
+}
+
+func cmDisableState(getCM func(name string) (v1alpha1.ConfigMap, error), source v1alpha1.ConfigMapDisableSource) (v1alpha1.DisableState, string, error) {
+	name := source.Name
+	key := source.Key
+	cm, err := getCM(name)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			return v1alpha1.DisableStatePending, fmt.Sprintf("ConfigMap %q does not exist", disableSource.ConfigMap.Name), nil
+			return v1alpha1.DisableStatePending, fmt.Sprintf("ConfigMap %q does not exist", name), nil
 		}
-		return v1alpha1.DisableStatePending, fmt.Sprintf("error reading ConfigMap %q", disableSource.ConfigMap.Name), err
+		return v1alpha1.DisableStatePending, fmt.Sprintf("error reading ConfigMap %q", name), err
 	}
 
-	cmVal, ok := cm.Data[disableSource.ConfigMap.Key]
+	cmVal, ok := cm.Data[key]
 	if !ok {
-		return v1alpha1.DisableStateError, fmt.Sprintf("ConfigMap %q has no key %q", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key), nil
+		return v1alpha1.DisableStateError, fmt.Sprintf("ConfigMap %q has no key %q", name, key), nil
 	}
+
 	isDisabled, err := strconv.ParseBool(cmVal)
 	if err != nil {
-		return v1alpha1.DisableStateError, fmt.Sprintf("error parsing ConfigMap/key %q/%q value %q as a bool: %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, cmVal, err.Error()), nil
+		return v1alpha1.DisableStateError, fmt.Sprintf("error parsing ConfigMap/key %q/%q value %q as a bool: %v", name, key, cmVal, err.Error()), nil
 	}
 
+	var result v1alpha1.DisableState
 	if isDisabled {
 		result = v1alpha1.DisableStateDisabled
 	} else {
 		result = v1alpha1.DisableStateEnabled
 	}
-	return result, fmt.Sprintf("ConfigMap/key %q/%q is %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, isDisabled), nil
+	return result, fmt.Sprintf("ConfigMap/key %q/%q is %v", name, key, isDisabled), nil
 }
 
 // Returns a new DisableStatus if the disable status has changed, or the prev status if it hasn't.

internal/controllers/apis/configmap/disable_test.go

@@ -15,6 +15,7 @@ import (
 )
 
 const configMapName = "fe-disable"
+const configMap2Name = "be-disable"
 const key = "isDisabled"
 
 func TestMaybeNewDisableStatusNoSource(t *testing.T) {
@@ -34,7 +35,7 @@ func TestMaybeNewDisableStatusNoConfigMapDisableSource(t *testing.T) {
 	require.NotNil(t, newStatus)
 	require.Equal(t, true, newStatus.Disabled)
 	require.Equal(t, v1alpha1.DisableStateError, newStatus.State)
-	require.Contains(t, newStatus.Reason, "specifies no ConfigMap")
+	require.Contains(t, newStatus.Reason, "specifies no valid sources")
 }
 
 func TestMaybeNewDisableStatusNoConfigMap(t *testing.T) {
@@ -80,6 +81,30 @@ func TestMaybeNewDisableStatusFalse(t *testing.T) {
 	require.Contains(t, newStatus.Reason, "is false")
 }
 
+func TestMaybeNewDisableStatusEveryTrue(t *testing.T) {
+	f := newDisableFixture(t)
+	f.createConfigMapNamed(configMapName, pointer.StringPtr("true"))
+	f.createConfigMapNamed(configMap2Name, pointer.StringPtr("true"))
+	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, everyDisableSource(), nil)
+	require.NoError(t, err)
+	require.NotNil(t, newStatus)
+	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateDisabled, newStatus.State)
+	require.Contains(t, newStatus.Reason, "Every ConfigMap disabled")
+}
+
+func TestMaybeNewDisableStatusEveryMixed(t *testing.T) {
+	f := newDisableFixture(t)
+	f.createConfigMapNamed(configMapName, pointer.StringPtr("true"))
+	f.createConfigMapNamed(configMap2Name, pointer.StringPtr("false"))
+	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, everyDisableSource(), nil)
+	require.NoError(t, err)
+	require.NotNil(t, newStatus)
+	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateEnabled, newStatus.State)
+	require.Contains(t, newStatus.Reason, "is false")
+}
+
 func TestMaybeNewDisableStatusGobbledygookValue(t *testing.T) {
 	f := newDisableFixture(t)
 	f.createConfigMap(pointer.StringPtr("asdf"))
@@ -124,13 +149,17 @@ type disableFixture struct {
 }
 
 func (f *disableFixture) createConfigMap(isDisabled *string) {
+	f.createConfigMapNamed(configMapName, isDisabled)
+}
+
+func (f *disableFixture) createConfigMapNamed(name string, isDisabled *string) {
 	m := make(map[string]string)
 	if isDisabled != nil {
 		m[key] = *isDisabled
 	}
 	err := f.fc.Create(f.ctx, &v1alpha1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: configMapName,
+			Name: name,
 		},
 		Data: m,
 	})
@@ -161,6 +190,21 @@ func disableSource() *v1alpha1.DisableSource {
 	}
 }
 
+func everyDisableSource() *v1alpha1.DisableSource {
+	return &v1alpha1.DisableSource{
+		EveryConfigMap: []v1alpha1.ConfigMapDisableSource{
+			{
+				Name: configMapName,
+				Key:  key,
+			},
+			{
+				Name: configMap2Name,
+				Key:  key,
+			},
+		},
+	}
+}
+
 func newDisableFixture(t *testing.T) *disableFixture {
 	fc := fake.NewFakeTiltClient()
 	ctx := context.Background()

internal/controllers/core/tiltfile/api.go

@@ -288,6 +288,38 @@ func toDisableSources(tlr *tiltfile.TiltfileLoadResult) disableSourceMap {
 	return result
 }
 
+func appendCMDS(cms []v1alpha1.ConfigMapDisableSource, newCM v1alpha1.ConfigMapDisableSource) []v1alpha1.ConfigMapDisableSource {
+	for _, cm := range cms {
+		if apicmp.DeepEqual(cm, newCM) {
+			return cms
+		}
+	}
+	return append(cms, newCM)
+}
+
+func mergeDisableSource(existing *v1alpha1.DisableSource, toMerge *v1alpha1.DisableSource) *v1alpha1.DisableSource {
+	if toMerge == nil {
+		return existing
+	}
+	if apicmp.DeepEqual(existing, toMerge) {
+		return existing
+	}
+
+	cms := []v1alpha1.ConfigMapDisableSource{}
+
+	if existing.ConfigMap != nil {
+		cms = append(cms, *existing.ConfigMap)
+	}
+	cms = append(cms, existing.EveryConfigMap...)
+	if toMerge.ConfigMap != nil {
+		cms = appendCMDS(cms, *toMerge.ConfigMap)
+	}
+	for _, newCM := range toMerge.EveryConfigMap {
+		cms = appendCMDS(cms, newCM)
+	}
+	return &v1alpha1.DisableSource{EveryConfigMap: cms}
+}
+
 func toDisableConfigMaps(disableSources disableSourceMap, enabledResources []model.ManifestName) apiset.TypedObjectSet {
 	enabledResourceSet := make(map[model.ManifestName]bool)
 	for _, mn := range enabledResources {
@@ -531,8 +563,12 @@ func toImageMapObjects(tlr *tiltfile.TiltfileLoadResult, disableSources disableS
 			}
 
 			name := iTarget.ImageMapName()
-			// Note that an ImageMap might be in more than one Manifest, so we
-			// can't annotate them to a particular manifest.
+			_, ok := result[name]
+			if ok {
+				// Some ImageTargets are shared among multiple manifests.
+				continue
+			}
+
 			im := &v1alpha1.ImageMap{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: name,

internal/controllers/core/tiltfile/api_test.go

@@ -164,6 +164,39 @@ func TestCmdImageCreate(t *testing.T) {
 	assert.Contains(t, ci.Spec.Ref, SanchoRef.String())
 }
 
+func TestTwoManifestsShareImage(t *testing.T) {
+	f := newAPIFixture(t)
+	target := model.MustNewImageTarget(SanchoRef).
+		WithBuildDetails(model.CustomBuild{
+			CmdImageSpec: v1alpha1.CmdImageSpec{Args: []string{"echo"}},
+			Deps:         []string{f.Path()},
+		})
+	fe1 := manifestbuilder.New(f, "fe1").
+		WithImageTarget(target).
+		WithK8sYAML(testyaml.SanchoYAML).
+		Build()
+	fe2 := manifestbuilder.New(f, "fe2").
+		WithImageTarget(target).
+		WithK8sYAML(testyaml.SanchoYAML).
+		Build()
+	nn := types.NamespacedName{Name: "tiltfile"}
+	tf := &v1alpha1.Tiltfile{ObjectMeta: metav1.ObjectMeta{Name: "tiltfile"}}
+	err := f.updateOwnedObjects(nn, tf,
+		&tiltfile.TiltfileLoadResult{Manifests: []model.Manifest{fe1, fe2}})
+	assert.NoError(t, err)
+
+	name := apis.SanitizeName(fe1.ImageTargets[0].ID().String())
+
+	var fw v1alpha1.FileWatch
+	assert.NoError(t, f.Get(types.NamespacedName{Name: name}, &fw))
+	assert.Equal(t, fw.Spec.DisableSource, &v1alpha1.DisableSource{
+		EveryConfigMap: []v1alpha1.ConfigMapDisableSource{
+			{Name: "fe1-disable", Key: "isDisabled"},
+			{Name: "fe2-disable", Key: "isDisabled"},
+		},
+	})
+}
+
 func TestAPITwoTiltfiles(t *testing.T) {
 	f := newAPIFixture(t)
 	feA := manifestbuilder.New(f, "fe-a").WithK8sYAML(testyaml.SanchoYAML).Build()

internal/controllers/core/tiltfile/filewatch.go

@@ -59,7 +59,7 @@ func addGlobalIgnoresToSpec(spec *v1alpha1.FileWatchSpec, globalIgnores []model.
 	}
 }
 
-// FileWatchesFromManifests creates FileWatch specs from Tilt manifests in the engine state.
+// Create FileWatch specs from Tilt manifests in the engine state.
 func ToFileWatchObjects(watchInputs WatchInputs, disableSources map[model.ManifestName]*v1alpha1.DisableSource) apiset.TypedObjectSet {
 	result := apiset.TypedObjectSet{}
 	if !watchInputs.EngineMode.WatchesFiles() {
@@ -68,23 +68,27 @@ func ToFileWatchObjects(watchInputs WatchInputs, disableSources map[model.Manife
 
 	// TODO(milas): how can global ignores fit into the API model more cleanly?
 	globalIgnores := globalIgnores(watchInputs)
-	var fileWatches []*v1alpha1.FileWatch
-	processedTargets := make(map[model.TargetID]bool)
 	for _, m := range watchInputs.Manifests {
 		for _, t := range m.TargetSpecs() {
 			targetID := t.ID()
 			// ignore targets that have already been processed or aren't watchable
-			_, seen := processedTargets[targetID]
 			t, ok := t.(WatchableTarget)
-			if seen || !ok || targetID.Empty() {
+			if !ok || targetID.Empty() {
 				continue
 			}
-			processedTargets[targetID] = true
+			name := apis.SanitizeName(targetID.String())
+			existing, ok := result[name]
+			if ok {
+				fw := existing.(*v1alpha1.FileWatch)
+				fw.Spec.DisableSource = mergeDisableSource(fw.Spec.DisableSource, disableSources[m.Name])
+				continue
+			}
+
 			spec := specForTarget(t, globalIgnores)
 			if spec != nil {
 				fw := &v1alpha1.FileWatch{
 					ObjectMeta: metav1.ObjectMeta{
-						Name: apis.SanitizeName(targetID.String()),
+						Name: name,
 						Annotations: map[string]string{
 							v1alpha1.AnnotationManifest: string(m.Name),
 							v1alpha1.AnnotationTargetID: targetID.String(),
@@ -93,7 +97,7 @@ func ToFileWatchObjects(watchInputs WatchInputs, disableSources map[model.Manife
 					Spec: *spec.DeepCopy(),
 				}
 				fw.Spec.DisableSource = disableSources[m.Name]
-				fileWatches = append(fileWatches, fw)
+				result[fw.Name] = fw
 			}
 		}
 	}
@@ -123,12 +127,9 @@ func ToFileWatchObjects(watchInputs WatchInputs, disableSources map[model.Manife
 		}
 
 		addGlobalIgnoresToSpec(&configFw.Spec, globalIgnores)
-		fileWatches = append(fileWatches, configFw)
+		result[configFw.Name] = configFw
 	}
 
-	for _, fw := range fileWatches {
-		result[fw.Name] = fw
-	}
 	return result
 }