rewriting unit tests

Signed-off-by: Joe Lanford <[email protected]>

Author: joelanford

.gitignore

@@ -28,6 +28,7 @@ vendor/
 
 # editor and IDE paraphernalia
 .idea/
+.run/
 *.swp
 *.swo
 *~

catalogd/internal/controllers/core/clustercatalog_controller_test.go

@@ -813,7 +813,7 @@ func TestPollingRequeue(t *testing.T) {
 			}
 			require.NoError(t, reconciler.setupFinalizers())
 			res, _ := reconciler.reconcile(context.Background(), tc.catalog)
-			assert.InDelta(t, tc.expectedRequeueAfter, res.RequeueAfter, requeueJitterMaxFactor*float64(tc.expectedRequeueAfter))
+			assert.InDelta(t, tc.expectedRequeueAfter, res.RequeueAfter, 2*requeueJitterMaxFactor*float64(tc.expectedRequeueAfter))
 		})
 	}
 }

internal/util/image/cache.go

@@ -39,31 +39,40 @@ const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
 
 func CatalogCache(basePath string) Cache {
 	return &diskCache{
-		basePath: basePath,
-		filterFunc: func(ctx context.Context, srcRef reference.Named, image ocispecv1.Image) (archive.Filter, error) {
-			_, specIsCanonical := srcRef.(reference.Canonical)
-			dirToUnpack, ok := image.Config.Labels[ConfigDirLabel]
-			if !ok {
-				// If the spec is a tagged ref, retries could end up resolving a new digest, where the label
-				// might show up. If the spec is canonical, no amount of retries will make the label appear.
-				// Therefore, we treat the error as terminal if the reference from the spec is canonical.
-				return nil, errorutil.WrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
-			}
+		basePath:   basePath,
+		filterFunc: filterForCatalogImage(),
+	}
+}
 
-			return allFilters(
-				onlyPath(dirToUnpack),
-				forceOwnershipRWX(),
-			), nil
-		},
+func filterForCatalogImage() func(ctx context.Context, srcRef reference.Named, image ocispecv1.Image) (archive.Filter, error) {
+	return func(ctx context.Context, srcRef reference.Named, image ocispecv1.Image) (archive.Filter, error) {
+		_, specIsCanonical := srcRef.(reference.Canonical)
+
+		dirToUnpack, ok := image.Config.Labels[ConfigDirLabel]
+		if !ok {
+			// If the spec is a tagged keep, retries could end up resolving a new digest, where the label
+			// might show up. If the spec is canonical, no amount of retries will make the label appear.
+			// Therefore, we treat the error as terminal if the reference from the spec is canonical.
+			return nil, errorutil.WrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
+		}
+
+		return allFilters(
+			onlyPath(dirToUnpack),
+			forceOwnershipRWX(),
+		), nil
 	}
 }
 
 func BundleCache(basePath string) Cache {
 	return &diskCache{
-		basePath: basePath,
-		filterFunc: func(_ context.Context, _ reference.Named, _ ocispecv1.Image) (archive.Filter, error) {
-			return forceOwnershipRWX(), nil
-		},
+		basePath:   basePath,
+		filterFunc: filterForBundleImage(),
+	}
+}
+
+func filterForBundleImage() func(ctx context.Context, srcRef reference.Named, image ocispecv1.Image) (archive.Filter, error) {
+	return func(ctx context.Context, srcRef reference.Named, image ocispecv1.Image) (archive.Filter, error) {
+		return forceOwnershipRWX(), nil
 	}
 }
 
@@ -111,26 +120,30 @@ func (a *diskCache) Store(ctx context.Context, ownerID string, srcRef reference.
 	if err := fsutil.EnsureEmptyDirectory(dest, 0700); err != nil {
 		return nil, time.Time{}, fmt.Errorf("error ensuring empty unpack directory: %w", err)
 	}
-	modTime, err := func() (time.Time, error) {
+
+	if err := func() error {
 		l := log.FromContext(ctx)
 		l.Info("unpacking image", "path", dest)
 		for layer := range layers {
 			if layer.Err != nil {
-				return time.Time{}, fmt.Errorf("error reading layer[%d]: %w", layer.Index, layer.Err)
+				return fmt.Errorf("error reading layer[%d]: %w", layer.Index, layer.Err)
 			}
 			if _, err := archive.Apply(ctx, dest, layer.Reader, applyOpts...); err != nil {
-				return time.Time{}, fmt.Errorf("error applying layer[%d]: %w", layer.Index, err)
+				return fmt.Errorf("error applying layer[%d]: %w", layer.Index, err)
 			}
 			l.Info("applied layer", "layer", layer.Index)
 		}
 		if err := fsutil.SetReadOnlyRecursive(dest); err != nil {
-			return time.Time{}, fmt.Errorf("error making unpack directory read-only: %w", err)
+			return fmt.Errorf("error making unpack directory read-only: %w", err)
 		}
-		return fsutil.GetDirectoryModTime(dest)
-	}()
-	if err != nil {
+		return nil
+	}(); err != nil {
 		return nil, time.Time{}, errors.Join(err, fsutil.DeleteReadOnlyRecursive(dest))
 	}
+	modTime, err := fsutil.GetDirectoryModTime(dest)
+	if err != nil {
+		return nil, time.Time{}, fmt.Errorf("error getting mod time of unpack directory: %w", err)
+	}
 	return os.DirFS(dest), modTime, nil
 }
 
@@ -142,17 +155,31 @@ func (a *diskCache) GarbageCollect(_ context.Context, ownerID string, keep refer
 	ownerIDPath := a.ownerIDPath(ownerID)
 	dirEntries, err := os.ReadDir(ownerIDPath)
 	if err != nil {
-		return fmt.Errorf("error reading image directories: %w", err)
+		if os.IsNotExist(err) {
+			return nil
+		}
+		return fmt.Errorf("error reading owner directory: %w", err)
 	}
 
+	foundKeep := false
 	dirEntries = slices.DeleteFunc(dirEntries, func(entry os.DirEntry) bool {
-		return entry.Name() == keep.Digest().String()
+		found := entry.Name() == keep.Digest().String()
+		if found {
+			foundKeep = true
+		}
+		return found
 	})
 
 	for _, dirEntry := range dirEntries {
 		if err := fsutil.DeleteReadOnlyRecursive(filepath.Join(ownerIDPath, dirEntry.Name())); err != nil {
 			return fmt.Errorf("error removing entry %s: %w", dirEntry.Name(), err)
 		}
 	}
+
+	if !foundKeep {
+		if err := fsutil.DeleteReadOnlyRecursive(ownerIDPath); err != nil {
+			return fmt.Errorf("error deleting unused owner data: %w", err)
+		}
+	}
 	return nil
 }