Remove versioner in favor of adding the function to the providers

This removes the `versioner` from the artifact ingester. Instead of
having this logic be part of the ingester, this simplifies things by
moving the details to the relevant providers.

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

internal/engine/ingester/artifact/artifact.go

@@ -28,6 +28,7 @@ import (
 
 	evalerrors "github.com/stacklok/minder/internal/engine/errors"
 	engif "github.com/stacklok/minder/internal/engine/interfaces"
+	artif "github.com/stacklok/minder/internal/providers/artifact"
 	"github.com/stacklok/minder/internal/verifier"
 	"github.com/stacklok/minder/internal/verifier/sigstore/container"
 	"github.com/stacklok/minder/internal/verifier/verifyif"
@@ -126,7 +127,7 @@ func (i *Ingest) getApplicableArtifactVersions(
 		return nil, err
 	}
 
-	vers, err := getVersioner(i.prov, artifact)
+	vers, err := getVersioner(i.prov)
 	if err != nil {
 		return nil, err
 	}
@@ -286,41 +287,24 @@ func getVerifier(i *Ingest, cfg *ingesterConfig) (verifyif.ArtifactVerifier, err
 func getAndFilterArtifactVersions(
 	ctx context.Context,
 	cfg *ingesterConfig,
-	vers versioner,
+	vers provifv1.ArtifactProvider,
 	artifact *pb.Artifact,
 ) ([]string, error) {
 	var res []string
 
 	// Build a tag filter based on the configuration
-	filter, err := buildTagMatcher(cfg.Tags, cfg.TagRegex)
+	filter, err := artif.BuildFilter(cfg.Tags, cfg.TagRegex)
 	if err != nil {
 		return nil, err
 	}
 
 	// Fetch all available versions of the artifact
-	upstreamVersions, err := vers.GetVersions(ctx)
+	upstreamVersions, err := vers.GetArtifactVersions(ctx, artifact, filter)
 	if err != nil {
 		return nil, fmt.Errorf("error retrieving artifact versions: %w", err)
 	}
 
-	name := artifact.GetName()
-
-	// Loop through all and filter out the versions that don't apply to this rule
 	for _, version := range upstreamVersions {
-		// Decide if the artifact version should be skipped or not
-		tags := version.GetTags()
-		tagsopt := map[string]interface{}{"tags": tags}
-		err = isSkippable(version.GetCreatedAt().AsTime(), tagsopt, filter)
-		if err != nil {
-			zerolog.Ctx(ctx).Debug().Str("name", name).Strs("tags", tags).Str(
-				"reason",
-				err.Error(),
-			).Msg("skipping artifact version")
-			continue
-		}
-
-		// If the artifact version is applicable to this rule, add it to the list
-		zerolog.Ctx(ctx).Debug().Str("name", name).Strs("tags", tags).Msg("artifact version matched")
 		res = append(res, version.Sha)
 	}
 
@@ -338,32 +322,6 @@ var (
 	ArtifactTypeContainerRetentionPeriod = time.Now().AddDate(0, -6, 0)
 )
 
-// isSkippable determines if an artifact should be skipped
-// Note this is only applicable to container artifacts.
-// TODO - this should be refactored as well, for now just a forklift from reconciler
-func isSkippable(createdAt time.Time, opts map[string]interface{}, filter tagMatcher) error {
-	// if the artifact is older than the retention period, skip it
-	if createdAt.Before(ArtifactTypeContainerRetentionPeriod) {
-		return fmt.Errorf("artifact is older than retention period - %s", ArtifactTypeContainerRetentionPeriod)
-	}
-	tags, ok := opts["tags"].([]string)
-	if !ok {
-		return nil
-	} else if len(tags) == 0 {
-		// if the artifact has no tags, skip it
-		return fmt.Errorf("artifact has no tags")
-	}
-	// if the artifact has a .sig tag it's a signature, skip it
-	if verifier.GetSignatureTag(tags) != "" {
-		return fmt.Errorf("artifact is a signature")
-	}
-	// if the artifact tags don't match the tag matcher, skip it
-	if !filter.MatchTag(tags...) {
-		return fmt.Errorf("artifact tags does not match")
-	}
-	return nil
-}
-
 func branchFromRef(ref string) string {
 	if strings.HasPrefix(ref, "refs/heads/") {
 		return ref[len("refs/heads/"):]
@@ -413,3 +371,12 @@ func signerIdentityFromCertificate(c *certificate.Summary) (string, error) {
 
 	return builderURL, nil
 }
+
+func getVersioner(prov provifv1.Provider) (provifv1.ArtifactProvider, error) {
+	ap, ok := prov.(provifv1.ArtifactProvider)
+	if !ok {
+		return nil, fmt.Errorf("provider does not implement ArtifactProvider")
+	}
+
+	return ap, nil
+}

internal/engine/ingester/artifact/tag_match.go → internal/providers/artifact/versionsfilter.go

@@ -1,4 +1,4 @@
-// Copyright 2023 Stacklok, Inc.
+// Copyright 2024 Stacklok, Inc.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -18,11 +18,16 @@ package artifact
 import (
 	"fmt"
 	"regexp"
+	"time"
 
+	"github.com/stacklok/minder/internal/verifier"
 	"k8s.io/apimachinery/pkg/util/sets"
+
+	provifv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
-func buildTagMatcher(tags []string, tagRegex string) (tagMatcher, error) {
+// BuildFilter builds a container image filter based on the tags and tag regex as well as the creation time
+func BuildFilter(tags []string, tagRegex string) (provifv1.GetArtifactVersionsFilter, error) {
 	if len(tags) > 0 && tagRegex != "" {
 		return nil, fmt.Errorf("cannot specify both tags and tag_regex")
 	}
@@ -33,7 +38,10 @@ func buildTagMatcher(tags []string, tagRegex string) (tagMatcher, error) {
 		if stags.HasAny("") {
 			return nil, fmt.Errorf("cannot specify empty tag")
 		}
-		return &tagListMatcher{tags: tags}, nil
+		return &filter{
+			tagMatcher:      &tagListMatcher{tags: tags},
+			retentionPeriod: provifv1.ArtifactTypeContainerRetentionPeriod,
+		}, nil
 	}
 
 	// no tags specified, but a regex was, compile it
@@ -42,13 +50,48 @@ func buildTagMatcher(tags []string, tagRegex string) (tagMatcher, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error compiling tag regex: %w", err)
 		}
-		return &tagRegexMatcher{re: re}, nil
+		return &filter{
+			tagMatcher:      &tagRegexMatcher{re: re},
+			retentionPeriod: provifv1.ArtifactTypeContainerRetentionPeriod,
+		}, nil
 	}
 
 	// no tags specified, match all
-	return &tagAllMatcher{}, nil
+	return &filter{
+		tagMatcher:      &tagAllMatcher{},
+		retentionPeriod: provifv1.ArtifactTypeContainerRetentionPeriod,
+	}, nil
+}
+
+type filter struct {
+	tagMatcher
+	retentionPeriod time.Time
+}
+
+// IsSkippable determines if an artifact should be skipped
+func (f *filter) IsSkippable(createdAt time.Time, tags []string) error {
+	// if the artifact is older than the retention period, skip it
+	if createdAt.Before(f.retentionPeriod) {
+		return fmt.Errorf("artifact is older than retention period - %s",
+			f.retentionPeriod)
+	}
+
+	if len(tags) == 0 {
+		// if the artifact has no tags, skip it
+		return fmt.Errorf("artifact has no tags")
+	}
+	// if the artifact has a .sig tag it's a signature, skip it
+	if verifier.GetSignatureTag(tags) != "" {
+		return fmt.Errorf("artifact is a signature")
+	}
+	// if the artifact tags don't match the tag matcher, skip it
+	if !f.MatchTag(tags...) {
+		return fmt.Errorf("artifact tags does not match")
+	}
+	return nil
 }
 
+// tagMatcher is an interface for matching tags
 type tagMatcher interface {
 	MatchTag(tags ...string) bool
 }

internal/providers/github/clients/app_test.go

@@ -116,28 +116,6 @@ func TestArtifactAPIEscapesApp(t *testing.T) {
 				assert.NoError(t, err)
 			},
 		},
-		{
-			name: "GetPackageVersions escapes the package name",
-			testHandler: func(w http.ResponseWriter, r *http.Request) {
-				assert.Equal(t, "/orgs/stacklok/packages/container/helm%2Fmediator/versions?package_type=container&page=1&per_page=100&state=active", r.URL.RequestURI())
-				w.WriteHeader(http.StatusOK)
-			},
-			cliFn: func(cli *github2.GitHub) {
-				_, err := cli.GetPackageVersions(context.Background(), "stacklok", "container", "helm/mediator")
-				assert.NoError(t, err)
-			},
-		},
-		{
-			name: "GetPackageVersionByTag escapes the package name",
-			testHandler: func(w http.ResponseWriter, r *http.Request) {
-				assert.Equal(t, "/orgs/stacklok/packages/container/helm%2Fmediator/versions?package_type=container&page=1&per_page=100&state=active", r.URL.RequestURI())
-				w.WriteHeader(http.StatusOK)
-			},
-			cliFn: func(cli *github2.GitHub) {
-				_, err := cli.GetPackageVersionByTag(context.Background(), "stacklok", "container", "helm/mediator", "v1.0.0")
-				assert.NoError(t, err)
-			},
-		},
 		{
 			name: "GetPackageVersionByID escapes the package name",
 			testHandler: func(w http.ResponseWriter, r *http.Request) {

internal/providers/github/clients/oauth_test.go

@@ -72,28 +72,6 @@ func TestArtifactAPIEscapesOAuth(t *testing.T) {
 				assert.NoError(t, err)
 			},
 		},
-		{
-			name: "GetPackageVersions escapes the package name",
-			testHandler: func(w http.ResponseWriter, r *http.Request) {
-				assert.Equal(t, "/orgs/stacklok/packages/container/helm%2Fmediator/versions?package_type=container&page=1&per_page=100&state=active", r.URL.RequestURI())
-				w.WriteHeader(http.StatusOK)
-			},
-			cliFn: func(cli *github2.GitHub) {
-				_, err := cli.GetPackageVersions(context.Background(), "stacklok", "container", "helm/mediator")
-				assert.NoError(t, err)
-			},
-		},
-		{
-			name: "GetPackageVersionByTag escapes the package name",
-			testHandler: func(w http.ResponseWriter, r *http.Request) {
-				assert.Equal(t, "/orgs/stacklok/packages/container/helm%2Fmediator/versions?package_type=container&page=1&per_page=100&state=active", r.URL.RequestURI())
-				w.WriteHeader(http.StatusOK)
-			},
-			cliFn: func(cli *github2.GitHub) {
-				_, err := cli.GetPackageVersionByTag(context.Background(), "stacklok", "container", "helm/mediator", "v1.0.0")
-				assert.NoError(t, err)
-			},
-		},
 		{
 			name: "GetPackageVersionByID escapes the package name",
 			testHandler: func(w http.ResponseWriter, r *http.Request) {