tiltfile: fix a bug where sometimes a base image wasn't pushed (#6514)

if a base image is used in ANY manifest,
it needs to be pushed in all manifests.
Otherwise the caching system will get confused.

fixes issue #6486

Signed-off-by: Nick Santos <[email protected]>

Author: nicks

internal/cli/docker_prune.go

@@ -100,10 +100,8 @@ func (c *dockerPruneCmd) run(ctx context.Context, args []string) error {
 // with all resources in a "disabled" state and rely on the API, but that's not
 // possible currently.
 func resolveImageSelectors(ctx context.Context, kCli k8s.Client, tlr *tiltfile.TiltfileLoadResult) ([]container.RefSelector, error) {
-	for _, m := range tlr.Manifests {
-		if err := m.InferImageProperties(); err != nil {
-			return nil, err
-		}
+	if err := model.InferImageProperties(tlr.Manifests); err != nil {
+		return nil, err
 	}
 
 	var reg *v1alpha1.RegistryHosting

internal/controllers/core/tiltfile/api.go

@@ -62,13 +62,13 @@ func updateOwnedObjects(
 	disableSources := toDisableSources(tlr)
 
 	if tlr != nil {
-		for i, m := range tlr.Manifests {
-			// Apply the registry to the image refs.
-			err := m.InferImageProperties()
-			if err != nil {
-				return err
-			}
+		// Apply the registry to the image refs.
+		err := model.InferImageProperties(tlr.Manifests)
+		if err != nil {
+			return err
+		}
 
+		for i, m := range tlr.Manifests {
 			// Assemble the LiveUpdate selectors, connecting objects together.
 			err = m.InferLiveUpdateSelectors()
 			if err != nil {

internal/controllers/core/tiltfile/reconciler_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"reflect"
+	"sort"
 	"strconv"
 	"testing"
 	"time"
@@ -455,6 +456,63 @@ func TestCancelClickedBeforeLoad(t *testing.T) {
 	require.Equal(t, "", tf.Status.Terminated.Error)
 }
 
+func TestPushBaseImageIssue6486(t *testing.T) {
+	f := newFixture(t)
+	p := f.tempdir.JoinPath("Tiltfile")
+
+	image1 := model.MustNewImageTarget(container.MustParseSelector("image-1")).
+		WithDockerImage(v1alpha1.DockerImageSpec{Context: f.tempdir.Path()})
+	image2 := model.MustNewImageTarget(container.MustParseSelector("image-2")).
+		WithDockerImage(v1alpha1.DockerImageSpec{Context: f.tempdir.Path()})
+	image3 := model.MustNewImageTarget(container.MustParseSelector("image-3")).
+		WithDockerImage(v1alpha1.DockerImageSpec{Context: f.tempdir.Path()}).
+		WithImageMapDeps([]string{"image-1", "image-2"})
+
+	service1 := manifestbuilder.New(f.tempdir, "service-1").
+		WithImageTargets(image1).
+		WithK8sYAML(testyaml.Deployment("service-1", "image-1")).
+		Build()
+	service3 := manifestbuilder.New(f.tempdir, "service-3").
+		WithImageTargets(image1, image2, image3).
+		WithK8sYAML(testyaml.Deployment("service-3", "image-3")).
+		Build()
+
+	f.tfl.Result = tiltfile.TiltfileLoadResult{
+		Manifests: []model.Manifest{service1, service3},
+	}
+
+	name := model.MainTiltfileManifestName.String()
+	tf := v1alpha1.Tiltfile{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+		},
+		Spec: v1alpha1.TiltfileSpec{
+			Path: p,
+		},
+	}
+	f.createAndWaitForLoaded(&tf)
+
+	assert.Equal(t, "", tf.Status.Terminated.Error)
+
+	var imageList = v1alpha1.DockerImageList{}
+	f.List(&imageList)
+
+	sort.Slice(imageList.Items, func(i, j int) bool {
+		return imageList.Items[i].Name < imageList.Items[j].Name
+	})
+
+	if assert.Equal(t, 4, len(imageList.Items)) {
+		assert.Equal(t, "service-1:image-1", imageList.Items[0].Name)
+		assert.Equal(t, v1alpha1.ClusterImageNeedsPush, imageList.Items[0].Spec.ClusterNeeds)
+		assert.Equal(t, "service-3:image-1", imageList.Items[1].Name)
+		assert.Equal(t, v1alpha1.ClusterImageNeedsPush, imageList.Items[1].Spec.ClusterNeeds)
+		assert.Equal(t, "service-3:image-2", imageList.Items[2].Name)
+		assert.Equal(t, v1alpha1.ClusterImageNeedsBase, imageList.Items[2].Spec.ClusterNeeds)
+		assert.Equal(t, "service-3:image-3", imageList.Items[3].Name)
+		assert.Equal(t, v1alpha1.ClusterImageNeedsPush, imageList.Items[3].Spec.ClusterNeeds)
+	}
+}
+
 type testStore struct {
 	*store.TestingStore
 	out *bytes.Buffer

internal/engine/upper_test.go

@@ -3850,9 +3850,7 @@ func (f *testFixture) setupDCFixture() (redis, server model.Manifest) {
 		f.T().Fatalf("Expected two manifests. Actual: %v", tlr.Manifests)
 	}
 
-	for _, m := range tlr.Manifests {
-		require.NoError(f.t, m.InferImageProperties())
-	}
+	require.NoError(f.t, model.InferImageProperties(tlr.Manifests))
 
 	return tlr.Manifests[0], tlr.Manifests[1]
 }

internal/testutils/manifestbuilder/manifestbuilder.go

@@ -232,7 +232,7 @@ func (b ManifestBuilder) Build() model.Manifest {
 	}
 	m = m.WithTriggerMode(b.triggerMode)
 
-	err := m.InferImageProperties()
+	err := model.InferImageProperties([]model.Manifest{m})
 	require.NoError(b.f.T(), err)
 
 	err = m.InferLiveUpdateSelectors()

internal/tiltfile/tiltfile_test.go

@@ -5947,11 +5947,7 @@ func (f *fixture) loadAllowWarnings(args ...string) {
 		f.t.Fatal(err)
 	}
 	f.loadResult = tlr
-
-	for _, m := range f.loadResult.Manifests {
-		err := m.InferImageProperties()
-		require.NoError(f.t, err)
-	}
+	require.NoError(f.t, model.InferImageProperties(f.loadResult.Manifests))
 }
 
 func unusedImageWarning(unusedImage string, suggestedImages []string, configType string) string {
@@ -5993,11 +5989,7 @@ func (f *fixture) loadArgsErrString(args []string, msgs ...string) {
 			f.t.Fatalf("error %q does not contain string %q", errText, msg)
 		}
 	}
-
-	for _, m := range f.loadResult.Manifests {
-		err := m.InferImageProperties()
-		require.NoError(f.t, err)
-	}
+	require.NoError(f.t, model.InferImageProperties(tlr.Manifests))
 }
 
 func (f *fixture) gitInit(path string) {

pkg/model/manifest.go

@@ -269,7 +269,7 @@ func (m *Manifest) ClusterName() string {
 }
 
 // Infer image properties for each image.
-func (m *Manifest) InferImageProperties() error {
+func (m *Manifest) inferImageProperties(clusterImageNeeds func(TargetID) v1alpha1.ClusterImageNeeds) error {
 	var deployImageIDs []TargetID
 	if m.DeployTarget != nil {
 		deployImageIDs = m.DeployTarget.DependencyIDs()
@@ -280,13 +280,8 @@ func (m *Manifest) InferImageProperties() error {
 	}
 
 	for i, iTarget := range m.ImageTargets {
-		// An image only needs to be pushed if it's used in-cluster.
-		clusterNeeds := v1alpha1.ClusterImageNeedsBase
-		if deployImageIDSet[iTarget.ID()] {
-			clusterNeeds = v1alpha1.ClusterImageNeedsPush
-		}
-
-		iTarget, err := iTarget.inferImageProperties(clusterNeeds, m.ClusterName())
+		iTarget, err := iTarget.inferImageProperties(
+			clusterImageNeeds(iTarget.ID()), m.ClusterName())
 		if err != nil {
 			return fmt.Errorf("manifest %s: %v", m.Name, err)
 		}
@@ -660,3 +655,32 @@ func equalForBuildInvalidation(x, y interface{}) bool {
 		ignoreRegistryFields,
 	)
 }
+
+// Infer image properties for each image in the manifest set.
+func InferImageProperties(manifests []Manifest) error {
+	deployImageIDSet := make(map[TargetID]bool, len(manifests))
+	for _, m := range manifests {
+		if m.DeployTarget != nil {
+			for _, depID := range m.DeployTarget.DependencyIDs() {
+				deployImageIDSet[depID] = true
+			}
+		}
+	}
+
+	// An image only needs to be pushed if it's used in-cluster.
+	// If it needs to be pushed for one manifest, it needs to be pushed for all.
+	// The caching system will make sure it's not pushed multiple times.
+	clusterImageNeeds := func(id TargetID) v1alpha1.ClusterImageNeeds {
+		if deployImageIDSet[id] {
+			return v1alpha1.ClusterImageNeedsPush
+		}
+		return v1alpha1.ClusterImageNeedsBase
+	}
+
+	for _, m := range manifests {
+		if err := m.inferImageProperties(clusterImageNeeds); err != nil {
+			return err
+		}
+	}
+	return nil
+}