Fix unnecessary lastUnpacked status updates (#397)

We should not be updating lastUnpacked status fields when we read from the cache Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
operator-framework · Sep 13, 2024 · dbdce9b · dbdce9b
1 parent d82e466
commit dbdce9b
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 3 deletions.
diff --git a/internal/source/image_registry_client.go b/internal/source/image_registry_client.go
@@ -75,8 +75,11 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
 	resolvedRef := fmt.Sprintf("%s@sha256:%s", imgRef.Context().Name(), digestHex)
 	if stat, err := os.Stat(unpackPath); err == nil && stat.IsDir() {
 		l.V(1).Info("found image in filesystem cache", "digest", digestHex)
-		// TODO: https://github.com/operator-framework/catalogd/issues/389
-		return unpackedResult(os.DirFS(unpackPath), catalog, resolvedRef, metav1.Time{Time: time.Now()}), nil
+		lastUnpacked := metav1.Time{Time: time.Now()}
+		if catalog.Status.ResolvedSource != nil && catalog.Status.ResolvedSource.Image != nil {
+			lastUnpacked = catalog.Status.ResolvedSource.Image.LastUnpacked
+		}
+		return unpackedResult(os.DirFS(unpackPath), catalog, resolvedRef, lastUnpacked), nil
 	}
 
 	if _, err = os.Stat(unpackPath); errors.Is(err, os.ErrNotExist) { //nolint: nestif

diff --git a/internal/source/image_registry_client_test.go b/internal/source/image_registry_client_test.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/go-logr/logr/funcr"
 	"github.com/google/go-containerregistry/pkg/name"
@@ -164,6 +165,13 @@ func TestImageRegistry(t *testing.T) {
 						},
 					},
 				},
+				Status: v1alpha1.ClusterCatalogStatus{
+					ResolvedSource: &v1alpha1.ResolvedCatalogSource{
+						Image: &v1alpha1.ResolvedImageSource{
+							LastUnpacked: metav1.Time{Time: time.Date(2000, 2, 1, 12, 30, 0, 0, time.UTC)},
+						},
+					},
+				},
 			},
 			wantErr: false,
 			image: func() v1.Image {
@@ -190,6 +198,13 @@ func TestImageRegistry(t *testing.T) {
 						},
 					},
 				},
+				Status: v1alpha1.ClusterCatalogStatus{
+					ResolvedSource: &v1alpha1.ResolvedCatalogSource{
+						Image: &v1alpha1.ResolvedImageSource{
+							LastUnpacked: metav1.Time{Time: time.Date(2000, 2, 1, 12, 30, 0, 1, time.UTC)},
+						},
+					},
+				},
 			},
 			wantErr:             false,
 			digestAlreadyExists: true,
@@ -216,6 +231,13 @@ func TestImageRegistry(t *testing.T) {
 						},
 					},
 				},
+				Status: v1alpha1.ClusterCatalogStatus{
+					ResolvedSource: &v1alpha1.ResolvedCatalogSource{
+						Image: &v1alpha1.ResolvedImageSource{
+							LastUnpacked: metav1.Time{Time: time.Date(2000, 2, 1, 12, 30, 0, 2, time.UTC)},
+						},
+					},
+				},
 			},
 			wantErr:         false,
 			oldDigestExists: true,
@@ -380,7 +402,15 @@ func TestImageRegistry(t *testing.T) {
 				assert.Len(t, entries, 1)
 				// If the digest should already exist check that we actually hit it
 				if tt.digestAlreadyExists {
-					require.Contains(t, buf.String(), "found image in filesystem cache")
+					assert.Contains(t, buf.String(), "found image in filesystem cache")
+					assert.Equal(t, tt.catalog.Status.ResolvedSource.Image.LastUnpacked, rs.ResolvedSource.Image.LastUnpacked)
+				} else if tt.oldDigestExists {
+					assert.NotContains(t, buf.String(), "found image in filesystem cache")
+					assert.NotEqual(t, tt.catalog.Status.ResolvedSource.Image.LastUnpacked, rs.ResolvedSource.Image.LastUnpacked)
+				} else {
+					require.NotNil(t, rs.ResolvedSource.Image.LastUnpacked)
+					require.NotNil(t, rs.ResolvedSource.Image)
+					assert.False(t, rs.ResolvedSource.Image.LastUnpacked.IsZero())
 				}
 			} else {
 				assert.Error(t, err)