chore: Operator / Feast should have matching Data Source types (#5041)

tchughesiv · web-flow · commit d937dcb7fc1e · 2025-02-10T16:27:32.000-06:00
data store types

Signed-off-by: Tommy Hughes &lt;tohughes@redhat.com&gt;
diff --git a/.github/workflows/operator_pr.yml b/.github/workflows/operator_pr.yml
@@ -11,8 +11,6 @@ jobs:
         with:
           go-version: 1.22.9
       - name: Operator tests
-        run: |
-          cd infra/feast-operator/
-          make test
+        run: make -C infra/feast-operator test
       - name: After code formatting, check for uncommitted differences
         run: git diff --exit-code infra/feast-operator
diff --git a/.github/workflows/pr_local_integration_tests.yml b/.github/workflows/pr_local_integration_tests.yml
@@ -56,3 +56,9 @@ jobs:
       - name: Test local integration tests
         if: ${{ always() }}  # this will guarantee that step won't be canceled and resources won't leak
         run: make test-python-integration-local
+      - name: Install Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: 1.22.9
+      - name: Operator Data Source types test
+        run: make -C infra/feast-operator test-datasources
diff --git a/infra/feast-operator/.golangci.yml b/infra/feast-operator/.golangci.yml
@@ -16,6 +16,9 @@ issues:
       linters:
         - dupl
         - lll
+    - path: "test/*"
+      linters:
+        - lll
 linters:
   disable-all: true
   enable:
diff --git a/infra/feast-operator/Makefile b/infra/feast-operator/Makefile
@@ -113,13 +113,19 @@ vet: ## Run go vet against code.
 
 .PHONY: test
 test: build-installer vet lint envtest ## Run tests.
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v test/e2e | grep -v test/data-source-types) -coverprofile cover.out
 
 # Utilize Kind or modify the e2e tests to load the image locally, enabling compatibility with other vendors.
 .PHONY: test-e2e  # Run the e2e tests against a Kind k8s instance that is spun up.
 test-e2e:
 	go test -timeout 30m ./test/e2e/ -v -ginkgo.v
 
+# Requires python3
+.PHONY: test-datasources
+test-datasources:
+	python3 test/data-source-types/data-source-types.py
+	go test ./test/data-source-types/
+
 .PHONY: lint
 lint: golangci-lint ## Run golangci-lint linter & yamllint
 	$(GOLANGCI_LINT) run
diff --git a/infra/feast-operator/api/v1alpha1/featurestore_types.go b/infra/feast-operator/api/v1alpha1/featurestore_types.go
@@ -114,8 +114,8 @@ var ValidOfflineStoreFilePersistenceTypes = []string{
 
 // OfflineStoreDBStorePersistence configures the DB store persistence for the offline store service
 type OfflineStoreDBStorePersistence struct {
-	// Type of the persistence type you want to use. Allowed values are: snowflake.offline, bigquery, redshift, spark, postgres, trino, redis, athena, mssql
-	// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;trino;redis;athena;mssql
+	// Type of the persistence type you want to use.
+	// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;trino;athena;mssql
 	Type string `json:"type"`
 	// Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed.
 	SecretRef corev1.LocalObjectReference `json:"secretRef"`
@@ -130,7 +130,6 @@ var ValidOfflineStoreDBStorePersistenceTypes = []string{
 	"spark",
 	"postgres",
 	"trino",
-	"redis",
 	"athena",
 	"mssql",
 }
@@ -160,7 +159,7 @@ type OnlineStoreFilePersistence struct {
 
 // OnlineStoreDBStorePersistence configures the DB store persistence for the online store service
 type OnlineStoreDBStorePersistence struct {
-	// Type of the persistence type you want to use. Allowed values are: snowflake.online, redis, ikv, datastore, dynamodb, bigtable, postgres, cassandra, mysql, hazelcast, singlestore, hbase, elasticsearch, qdrant, couchbase, milvus
+	// Type of the persistence type you want to use.
 	// +kubebuilder:validation:Enum=snowflake.online;redis;ikv;datastore;dynamodb;bigtable;postgres;cassandra;mysql;hazelcast;singlestore;hbase;elasticsearch;qdrant;couchbase;milvus
 	Type string `json:"type"`
 	// Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed.
@@ -215,7 +214,7 @@ type RegistryFilePersistence struct {
 
 // RegistryDBStorePersistence configures the DB store persistence for the registry service
 type RegistryDBStorePersistence struct {
-	// Type of the persistence type you want to use. Allowed values are: sql, snowflake.registry
+	// Type of the persistence type you want to use.
 	// +kubebuilder:validation:Enum=sql;snowflake.registry
 	Type string `json:"type"`
 	// Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed.
@@ -225,8 +224,8 @@ type RegistryDBStorePersistence struct {
 }
 
 var ValidRegistryDBStorePersistenceTypes = []string{
-	"snowflake.registry",
 	"sql",
+	"snowflake.registry",
 }
 
 // PvcConfig defines the settings for a persistent file store based on PVCs.
diff --git a/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml b/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml
@@ -170,7 +170,6 @@ spec:
                                 - spark
                                 - postgres
                                 - trino
-                                - redis
                                 - athena
                                 - mssql
                                 type: string
@@ -2083,7 +2082,6 @@ spec:
                                     - spark
                                     - postgres
                                     - trino
-                                    - redis
                                     - athena
                                     - mssql
                                     type: string
diff --git a/infra/feast-operator/dist/install.yaml b/infra/feast-operator/dist/install.yaml
@@ -178,7 +178,6 @@ spec:
                                 - spark
                                 - postgres
                                 - trino
-                                - redis
                                 - athena
                                 - mssql
                                 type: string
@@ -2091,7 +2090,6 @@ spec:
                                     - spark
                                     - postgres
                                     - trino
-                                    - redis
                                     - athena
                                     - mssql
                                     type: string
diff --git a/infra/feast-operator/docs/api/markdown/ref.md b/infra/feast-operator/docs/api/markdown/ref.md
@@ -210,7 +210,7 @@ _Appears in:_
 
 | Field | Description |
 | --- | --- |
-| `type` _string_ | Type of the persistence type you want to use. Allowed values are: snowflake.offline, bigquery, redshift, spark, postgres, trino, redis, athena, mssql |
+| `type` _string_ | Type of the persistence type you want to use. |
 | `secretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#localobjectreference-v1-core)_ | Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed. |
 | `secretKeyName` _string_ | By default, the selected store "type" is used as the SecretKeyName |
 
@@ -286,7 +286,7 @@ _Appears in:_
 
 | Field | Description |
 | --- | --- |
-| `type` _string_ | Type of the persistence type you want to use. Allowed values are: snowflake.online, redis, ikv, datastore, dynamodb, bigtable, postgres, cassandra, mysql, hazelcast, singlestore, hbase, elasticsearch, qdrant, couchbase, milvus |
+| `type` _string_ | Type of the persistence type you want to use. |
 | `secretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#localobjectreference-v1-core)_ | Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed. |
 | `secretKeyName` _string_ | By default, the selected store "type" is used as the SecretKeyName |
 
diff --git a/infra/feast-operator/test/api/featurestore_types_test.go b/infra/feast-operator/test/api/featurestore_types_test.go
@@ -2,6 +2,8 @@ package api
 
 import (
 	"context"
+	"fmt"
+	"strings"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -334,6 +336,16 @@ func registryStoreWithDBPersistenceType(dbPersistenceType string, featureStore *
 	return fsCopy
 }
 
+func quotedSlice(stringSlice []string) string {
+	quotedSlice := make([]string, len(stringSlice))
+
+	for i, str := range stringSlice {
+		quotedSlice[i] = fmt.Sprintf("\"%s\"", str)
+	}
+
+	return strings.Join(quotedSlice, ", ")
+}
+
 const resourceName = "test-resource"
 const namespaceName = "default"
 
@@ -377,7 +389,7 @@ var _ = Describe("FeatureStore API", func() {
 		})
 
 		It("should fail when db persistence type is invalid", func() {
-			attemptInvalidCreationAndAsserts(ctx, onlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"snowflake.online\", \"redis\", \"ikv\", \"datastore\", \"dynamodb\", \"bigtable\", \"postgres\", \"cassandra\", \"mysql\", \"hazelcast\", \"singlestore\", \"hbase\", \"elasticsearch\", \"qdrant\", \"couchbase\"")
+			attemptInvalidCreationAndAsserts(ctx, onlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidOnlineStoreDBStorePersistenceTypes))
 		})
 	})
 
@@ -388,7 +400,7 @@ var _ = Describe("FeatureStore API", func() {
 			attemptInvalidCreationAndAsserts(ctx, offlineStoreWithUnmanagedFileType(featurestore), "Unsupported value")
 		})
 		It("should fail when db persistence type is invalid", func() {
-			attemptInvalidCreationAndAsserts(ctx, offlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"snowflake.offline\", \"bigquery\", \"redshift\", \"spark\", \"postgres\", \"trino\", \"redis\", \"athena\", \"mssql\"")
+			attemptInvalidCreationAndAsserts(ctx, offlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidOfflineStoreDBStorePersistenceTypes))
 		})
 	})
 
@@ -410,7 +422,7 @@ var _ = Describe("FeatureStore API", func() {
 			attemptInvalidCreationAndAsserts(ctx, registryWithS3AdditionalKeywordsForGsBucket(featurestore), "Additional S3 settings are available only for S3 object store URIs")
 		})
 		It("should fail when db persistence type is invalid", func() {
-			attemptInvalidCreationAndAsserts(ctx, registryStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"sql\", \"snowflake.registry\"")
+			attemptInvalidCreationAndAsserts(ctx, registryStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidRegistryDBStorePersistenceTypes))
 		})
 	})
 
diff --git a/infra/feast-operator/test/data-source-types/data-source-types.py b/infra/feast-operator/test/data-source-types/data-source-types.py
@@ -0,0 +1,18 @@
+import os
+from feast.repo_config import REGISTRY_CLASS_FOR_TYPE, OFFLINE_STORE_CLASS_FOR_TYPE, ONLINE_STORE_CLASS_FOR_TYPE, LEGACY_ONLINE_STORE_CLASS_FOR_TYPE
+
+def save_in_script_directory(filename: str, typedict: dict[str, str]):
+    script_dir = os.path.dirname(os.path.abspath(__file__))
+    file_path = os.path.join(script_dir, filename)
+    
+    with open(file_path, 'w') as file:
+        for k in typedict.keys():
+            file.write(k+"\n")
+
+for legacyType in LEGACY_ONLINE_STORE_CLASS_FOR_TYPE.keys():
+    if legacyType in ONLINE_STORE_CLASS_FOR_TYPE:
+        del ONLINE_STORE_CLASS_FOR_TYPE[legacyType]
+
+save_in_script_directory("registry.out", REGISTRY_CLASS_FOR_TYPE)
+save_in_script_directory("online-store.out", ONLINE_STORE_CLASS_FOR_TYPE)
+save_in_script_directory("offline-store.out", OFFLINE_STORE_CLASS_FOR_TYPE)
diff --git a/infra/feast-operator/test/data-source-types/data_source_types_test.go b/infra/feast-operator/test/data-source-types/data_source_types_test.go
@@ -0,0 +1,88 @@
+package datasources
+
+import (
+	"bufio"
+	"os"
+	"slices"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	feastdevv1alpha1 "github.com/feast-dev/feast/infra/feast-operator/api/v1alpha1"
+	"github.com/feast-dev/feast/infra/feast-operator/internal/controller/services"
+)
+
+func TestDataSourceTypes(t *testing.T) {
+	RegisterFailHandler(Fail)
+
+	RunSpecs(t, "Data Source Suite")
+}
+
+var _ = Describe("FeatureStore Data Source Types", func() {
+	Context("When checking against the python code in feast.repo_config", func() {
+		It("should match defined registry persistence types in the operator", func() {
+			registryFilePersistenceTypes := []string{string(services.RegistryFileConfigType)}
+			registryPersistenceTypes := append(feastdevv1alpha1.ValidRegistryDBStorePersistenceTypes, registryFilePersistenceTypes...)
+			checkPythonPersistenceTypes("registry.out", registryPersistenceTypes)
+		})
+		It("should match defined onlineStore persistence types in the operator", func() {
+			onlineFilePersistenceTypes := []string{string(services.OnlineSqliteConfigType)}
+			onlinePersistenceTypes := append(feastdevv1alpha1.ValidOnlineStoreDBStorePersistenceTypes, onlineFilePersistenceTypes...)
+			checkPythonPersistenceTypes("online-store.out", onlinePersistenceTypes)
+		})
+		It("should match defined offlineStore persistence types in the operator", func() {
+			offlinePersistenceTypes := append(feastdevv1alpha1.ValidOfflineStoreDBStorePersistenceTypes, feastdevv1alpha1.ValidOfflineStoreFilePersistenceTypes...)
+			checkPythonPersistenceTypes("offline-store.out", offlinePersistenceTypes)
+		})
+	})
+})
+
+func checkPythonPersistenceTypes(fileName string, operatorDsTypes []string) {
+	feastDsTypes, err := readFileLines(fileName)
+	Expect(err).NotTo(HaveOccurred())
+
+	// Add remote type to slice, as its not a file or db type and we want to limit its use to registry service when deploying with the operator
+	operatorDsTypes = append(operatorDsTypes, "remote")
+	missingFeastTypes := []string{}
+	for _, ods := range operatorDsTypes {
+		if len(ods) > 0 {
+			if !slices.Contains(feastDsTypes, ods) {
+				missingFeastTypes = append(missingFeastTypes, ods)
+			}
+		}
+	}
+	Expect(missingFeastTypes).To(BeEmpty())
+
+	missingOperatorTypes := []string{}
+	for _, fds := range feastDsTypes {
+		if len(fds) > 0 {
+			if !slices.Contains(operatorDsTypes, fds) {
+				missingOperatorTypes = append(missingOperatorTypes, fds)
+			}
+		}
+	}
+	Expect(missingOperatorTypes).To(BeEmpty())
+}
+
+func readFileLines(filePath string) ([]string, error) {
+	file, err := os.Open(filePath)
+	Expect(err).NotTo(HaveOccurred())
+	defer closeFile(file)
+
+	var lines []string
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		lines = append(lines, scanner.Text())
+	}
+
+	err = scanner.Err()
+	Expect(err).NotTo(HaveOccurred())
+
+	return lines, nil
+}
+
+func closeFile(file *os.File) {
+	err := file.Close()
+	Expect(err).NotTo(HaveOccurred())
+}
