Skip to content

chore: make CM content aligned to current EH Service dev#644

Closed
tarilabs wants to merge 3 commits intotrustyai-explainability:mainfrom
tarilabs:tarilabs-20260213-similarConfigMap
Closed

chore: make CM content aligned to current EH Service dev#644
tarilabs wants to merge 3 commits intotrustyai-explainability:mainfrom
tarilabs:tarilabs-20260213-similarConfigMap

Conversation

@tarilabs
Copy link
Member

@tarilabs tarilabs commented Feb 13, 2026

Introduce a evalhub-providers ConfigMap with 1 entry per providers/<name>.yaml
To resemble current EH Service config/ directory content
in this case, we need 1 provider yaml per config/providers subdirectory.

We are still evaluating how to add "additional user-provided providers",
and likely we will need 1 Provider 1 ConfigMap with annotation (or a CRD)

But with this PR I make the content of /etc/evalhub
Similar to /app/config/providers

Later, we can avoid harcoded value on Deployment creation of the CM, and just mount the ootb ConfigMaps and User-provided-providers ConfigMaps, this could be 1 step filling the gap

I avoid introducing dependency to eval-hub repo in go.mod (for now).

DEMO

Screenshot 2026-02-13 at 20 52 35 Screenshot 2026-02-13 at 20 53 14 Screenshot 2026-02-13 at 20 54 36

Summary by Sourcery

Align EvalHub controller ConfigMap generation and tests with the latest Eval Hub service provider API and configuration layout.

New Features:

  • Generate one YAML file per provider under a providers/ directory instead of a single aggregated providers.yaml.
  • Introduce local copies of ProviderResource, BenchmarkResource, and related runtime types mirroring the Eval Hub API.

Enhancements:

  • Update default provider definitions to use structured benchmark resources compatible with the new provider schema.
  • Adjust controller deployment configuration to point PROVIDERS_CONFIG_PATH to the providers/ directory rather than a single file.

Tests:

  • Update controller, configmap, unit, and integration tests to validate per-provider YAML files and the new provider data structures instead of providers.yaml.

Summary by CodeRabbit

Release Notes

  • Refactor
    • Provider configurations are now managed as individual YAML files in a dedicated configuration directory instead of a single combined file
    • Updated configuration directory structure from file-based to directory-based organization for provider settings

@openshift-ci
Copy link

openshift-ci bot commented Feb 13, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link

openshift-ci bot commented Feb 13, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Feb 13, 2026

Reviewer's Guide

Splits provider configuration into a dedicated providers ConfigMap mounted as a directory of per-provider YAML files, updates the EvalHub controller and deployment to use Eval Hub API-aligned provider types and layout, and adjusts tests accordingly.

Sequence diagram for reconciling the providers ConfigMap

sequenceDiagram
    participant Reconciler as EvalHubReconciler
    participant K8sAPI as KubernetesAPIServer
    participant ProvidersCM as ProvidersConfigMap

    Reconciler->>K8sAPI: Get EvalHub instance
    K8sAPI-->>Reconciler: EvalHub instance

    Reconciler->>Reconciler: buildEvalHubConfig(instance)
    Reconciler->>Reconciler: generateProvidersData(config.Providers)

    Reconciler->>K8sAPI: Get ConfigMap name=instance-name-providers
    alt ConfigMap not found
        K8sAPI-->>Reconciler: NotFound
        Reconciler->>ProvidersCM: Set Data = per-provider YAML
        Reconciler->>ProvidersCM: SetControllerReference(instance)
        Reconciler->>K8sAPI: Create Providers ConfigMap
        K8sAPI-->>Reconciler: Created
    else ConfigMap exists
        K8sAPI-->>Reconciler: Existing ConfigMap
        Reconciler->>ProvidersCM: Update Data = per-provider YAML
        Reconciler->>K8sAPI: Update Providers ConfigMap
        K8sAPI-->>Reconciler: Updated
    end

    Reconciler-->>Reconciler: Continue reconciliation (Deployment, Proxy CM, etc.)
Loading

Class diagram for updated EvalHub provider configuration types

classDiagram
    class EvalHubConfig {
        +[]ProviderResource Providers
        +[]string Collections
        +*DatabaseConfig Database
        +*SecretsMapping Secrets
    }

    class DatabaseConfig {
        +string Driver
        +string Host
        +int Port
        +string User
        +string Password
        +string Name
        +int MaxOpenConns
        +int MaxIdleConns
    }

    class SecretsMapping {
        +string DatabaseUserSecret
        +string DatabasePasswordSecret
    }

    class ProviderResource {
        +string ID
        +string Name
        +string Description
        +string Type
        +[]BenchmarkResource Benchmarks
        +*Runtime Runtime
    }

    class BenchmarkResource {
        +string ID
        +*string ProviderId
        +string Name
        +string Description
        +string Category
        +[]string Metrics
        +int NumFewShot
        +int DatasetSize
        +[]string Tags
    }

    class Runtime {
        +*K8sRuntime K8s
        +*LocalRuntime Local
    }

    class K8sRuntime {
        +string Image
        +[]string Entrypoint
        +string CPURequest
        +string MemoryRequest
        +string CPULimit
        +string MemoryLimit
        +[]EnvVar Env
    }

    class LocalRuntime {
    }

    class EnvVar {
        +string Name
        +string Value
    }

    EvalHubConfig --> "*" ProviderResource : providers
    EvalHubConfig --> "0..1" DatabaseConfig : database
    EvalHubConfig --> "0..1" SecretsMapping : secrets
    ProviderResource --> "*" BenchmarkResource : benchmarks
    ProviderResource --> "0..1" Runtime : runtime
    Runtime --> "0..1" K8sRuntime : k8s
    Runtime --> "0..1" LocalRuntime : local
    K8sRuntime --> "*" EnvVar : env
Loading

File-Level Changes

Change Details Files
Refactor EvalHub configuration generation to use Eval Hub-compatible ProviderResource/BenchmarkResource types and separate building of the base config from serialization.
  • Replace ProviderConfig with ProviderResource in EvalHubConfig and update default provider definitions to use BenchmarkResource slices instead of string IDs and config maps.
  • Introduce buildEvalHubConfig to construct EvalHubConfig (providers, collections, optional database) independently of YAML marshaling.
  • Update generateConfigData to only marshal EvalHubConfig into config.yaml and stop embedding providers.yaml in the main ConfigMap.
controllers/evalhub/configmap.go
controllers/evalhub/configmap_test.go
controllers/evalhub/unit_test.go
Introduce a separate providers ConfigMap containing one YAML document per provider and wire it into reconciliation and deployment.
  • Add generateProvidersData to emit a map of .yaml -> ProviderResource YAML and reconcileProvidersConfigMap to manage a -providers ConfigMap.
  • Call reconcileProvidersConfigMap from the main Reconcile loop and ensure controller-owner references are set when creating the providers ConfigMap.
  • Mount a new evalhub-providers volume from the providers ConfigMap into /etc/evalhub/providers, update PROVIDERS_CONFIG_PATH to point to the directory, and adjust volume/volumeMount counts in tests.
controllers/evalhub/configmap.go
controllers/evalhub/deployment.go
controllers/evalhub/evalhub_controller.go
controllers/evalhub/unit_test.go
controllers/evalhub/deployment_test.go
controllers/evalhub/evalhub_controller_test.go
Align provider-related type definitions and tests with the Eval Hub service provider API model.
  • Add provider_types.go with local copies of ProviderResource, BenchmarkResource, Runtime, K8sRuntime, LocalRuntime, and EnvVar mirroring eval-hub/pkg/api.
  • Update tests to work with BenchmarkResource slices (using helper functions to extract benchmark names) and remove assertions around deprecated ProviderConfig fields like Enabled and Config.
  • Change tests that previously validated a single providers.yaml entry in the main ConfigMap to instead validate the per-provider files in the separate providers ConfigMap and expect updated volume/volumeMount layouts.
controllers/evalhub/provider_types.go
controllers/evalhub/configmap_test.go
controllers/evalhub/unit_test.go
controllers/evalhub/deployment_test.go
controllers/evalhub/evalhub_controller_test.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

📝 Walkthrough

Walkthrough

The PR refactors EvalHub provider configuration from hard-coded ProviderConfig entries to a ProviderResource model, builds an in-memory EvalHubConfig via a new builder, emits per-provider YAML into a dedicated Providers ConfigMap, and mounts that ConfigMap as a directory in the EvalHub deployment.

Changes

Cohort / File(s) Summary
New Provider Resource Types
controllers/evalhub/provider_types.go
Added exported types: ProviderResource, BenchmarkResource, Runtime, K8sRuntime, LocalRuntime, and EnvVar (serialization tags included).
ConfigMap Reconciliation Refactoring
controllers/evalhub/configmap.go, controllers/evalhub/configmap_test.go
Replaced ProviderConfig with ProviderResource. Added buildEvalHubConfig, generateProvidersData, and reconcileProvidersConfigMap. Config generation now emits a single config.yaml and a separate providers ConfigMap with per-provider YAML keys; tests updated and helper benchmarkResourceNames added.
Deployment Configuration Updates
controllers/evalhub/deployment.go, controllers/evalhub/deployment_test.go
Changed PROVIDERS_CONFIG_PATH from "/etc/evalhub/providers.yaml" to "/etc/evalhub/providers/". Added evalhub-providers volume/volumeMount backed by <instance>-providers ConfigMap; tests updated for extra volumes/mounts.
Controller Reconciliation Flow
controllers/evalhub/evalhub_controller.go, controllers/evalhub/evalhub_controller_test.go, controllers/evalhub/unit_test.go
Inserted providers ConfigMap reconciliation step into main reconcile flow with error handling/status updates. Tests extended to wait for and validate the providers ConfigMap and its per-provider keys.

Sequence Diagram(s)

sequenceDiagram
  participant Reconciler as Reconciler (EvalHub)
  participant Builder as buildEvalHubConfig
  participant ProvidersCM as Providers ConfigMap
  participant K8sAPI as Kubernetes API
  participant Deployment as Deployment (Pod)

  Reconciler->>Builder: buildEvalHubConfig(instance)
  Builder-->>Reconciler: EvalHubConfig (with ProviderResource entries)
  Reconciler->>ProvidersCM: reconcileProvidersConfigMap(EvalHubConfig.Providers)
  ProvidersCM->>K8sAPI: create/update ConfigMap keys (per-provider YAML)
  K8sAPI-->>ProvidersCM: ack
  Reconciler->>Deployment: ensure volume/volumeMount for providers dir
  Deployment->>K8sAPI: patch/create Deployment with evalhub-providers volume
  K8sAPI-->>Deployment: ack
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels

kind/enhancement, lgtm

Suggested reviewers

  • ruivieira
  • RobGeada
  • julpayne

Poem

🐇 I stitched each provider into a row,
YAML petals in a folder to show;
Reconciler hums, mounts them in place,
Pods read each file with neat little grace.
Hooray — configs hop forward, one per case!

🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'chore: make CM content aligned to current EH Service dev' is vague and uses non-descriptive terms that don't clearly convey the main change, making it difficult for someone scanning history to understand what was modified. Revise the title to be more specific and descriptive, such as 'refactor: separate provider configs into dedicated ConfigMap with per-provider YAML files' to clearly indicate the primary structural change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
controllers/evalhub/configmap.go (2)

82-158: Duplicate buildEvalHubConfig invocations during reconciliation.

buildEvalHubConfig is called once in generateConfigData (line 162) and again in reconcileProvidersConfigMap (line 207). If both reconcileConfigMap and reconcileProvidersConfigMap are invoked during the same reconcile loop, the identical config is built twice. Consider building it once in the caller and passing it down.

Additionally, the providers list ends up in both config.yaml (via the main ConfigMap) and as individual per-provider YAML files (via the providers ConfigMap). If the Eval Hub service reads providers from PROVIDERS_CONFIG_PATH (the per-provider directory), the embedded providers array in config.yaml is redundant and could diverge if one ConfigMap update succeeds but the other fails.

♻️ Suggested refactor: build config once, optionally strip providers from config.yaml
-func (r *EvalHubReconciler) generateConfigData(instance *evalhubv1alpha1.EvalHub) (map[string]string, error) {
-	config := r.buildEvalHubConfig(instance)
-
-	configYAML, err := yaml.Marshal(config)
+func (r *EvalHubReconciler) generateConfigData(config EvalHubConfig) (map[string]string, error) {
+	// Omit providers from the main config since they are served via the providers ConfigMap
+	configForMain := config
+	configForMain.Providers = nil
+
+	configYAML, err := yaml.Marshal(configForMain)
 	if err != nil {
 		return nil, err
 	}
 
 	return map[string]string{
 		"config.yaml": string(configYAML),
 	}, nil
 }

Then in the reconcile caller, build once and pass to both:

config := r.buildEvalHubConfig(instance)
configData, err := r.generateConfigData(config)
// ...
providersData, err := r.generateProvidersData(config.Providers)

174-186: LGTM — clean per-provider marshaling helper.

The iteration and error handling look correct. One note: the BenchmarkResource struct fields (description, category, metrics, num_few_shot, dataset_size, tags) lack omitempty in their YAML struct tags, which means generated YAML will include zero-value noise (empty strings, 0, empty arrays) for every benchmark. However, since these types are copied from upstream (github.com/eval-hub/eval-hub/pkg/api), consider whether to modify them in sync with the source.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tarilabs tarilabs added project/evalhub needs-orchestrator-build Pull requests that need a new build of the Guardrails Orchestrator for CI and removed needs-orchestrator-build Pull requests that need a new build of the Guardrails Orchestrator for CI labels Feb 13, 2026
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
@tarilabs tarilabs force-pushed the tarilabs-20260213-similarConfigMap branch from 935030e to 1c357b3 Compare February 13, 2026 17:45
@tarilabs tarilabs marked this pull request as ready for review February 13, 2026 19:49
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • In generateProvidersData you key the ConfigMap entries off provider.Name, but ProviderResource already has an ID field that is more stable and less likely to contain characters invalid in filenames; consider switching to ID (and/or sanitizing the chosen field) for the <name>.yaml key to avoid subtle issues if names change or contain path-unfriendly characters.
  • The default provider/benchmark definitions are now embedded directly in buildEvalHubConfig and mirrored in several tests via hard-coded names; consider centralizing these defaults in a shared constant or helper so provider changes only need to be made in one place and tests can reference them symbolically.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `generateProvidersData` you key the ConfigMap entries off `provider.Name`, but `ProviderResource` already has an `ID` field that is more stable and less likely to contain characters invalid in filenames; consider switching to `ID` (and/or sanitizing the chosen field) for the `<name>.yaml` key to avoid subtle issues if names change or contain path-unfriendly characters.
- The default provider/benchmark definitions are now embedded directly in `buildEvalHubConfig` and mirrored in several tests via hard-coded names; consider centralizing these defaults in a shared constant or helper so provider changes only need to be made in one place and tests can reference them symbolically.

## Individual Comments

### Comment 1
<location> `controllers/evalhub/configmap.go:174-186` </location>
<code_context>
+// generateProvidersData generates per-provider YAML entries for the providers ConfigMap.
+// Each provider becomes a separate key like "lm-eval-harness.yaml".
+func (r *EvalHubReconciler) generateProvidersData(providers []ProviderResource) (map[string]string, error) {
+	data := make(map[string]string, len(providers))
+	for _, provider := range providers {
+		providerYAML, err := yaml.Marshal(provider)
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal provider %s: %w", provider.Name, err)
+		}
+		data[fmt.Sprintf("%s.yaml", provider.Name)] = string(providerYAML)
+	}
+	return data, nil
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Use provider ID (or ID with fallback) as the filename key instead of Name.

Using `provider.Name` here couples the ConfigMap key/filename to a mutable, user-facing field that may be non-unique or contain characters invalid for keys. Since `ProviderResource` has a stable `ID`, prefer `provider.ID` for the key (e.g. `fmt.Sprintf("%s.yaml", provider.ID)`), or fall back to `Name` only if `ID` is empty, to avoid collisions and unintended breakage when display names change.

```suggestion
// generateProvidersData generates per-provider YAML entries for the providers ConfigMap.
// Each provider becomes a separate key like "provider-id.yaml", preferring the stable provider ID.
func (r *EvalHubReconciler) generateProvidersData(providers []ProviderResource) (map[string]string, error) {
	data := make(map[string]string, len(providers))
	for _, provider := range providers {
		key := provider.ID
		if key == "" {
			key = provider.Name
		}

		providerYAML, err := yaml.Marshal(provider)
		if err != nil {
			return nil, fmt.Errorf("failed to marshal provider %s: %w", key, err)
		}
		data[fmt.Sprintf("%s.yaml", key)] = string(providerYAML)
	}
	return data, nil
}
```
</issue_to_address>

### Comment 2
<location> `controllers/evalhub/configmap_test.go:132-141` </location>
<code_context>
+		It("should have valid per-provider YAML files in providers ConfigMap", func() {
</code_context>

<issue_to_address>
**suggestion (testing):** Strengthen providers ConfigMap test by asserting exact keys and expected provider shapes (type and benchmarks) rather than only non-empty name

Currently the test only checks that four expected keys exist and that each unmarshalled `ProviderResource` has a non-empty `Name`. To better match the new provider schema, tighten the assertions by:
- Verifying `configMap.Data` contains exactly the expected provider keys (e.g. `HaveLen(4)` and explicit key set).
- After unmarshalling, asserting `provider.Type` and the expected benchmark names per provider (using `benchmarkResourceNames`), not just a non-empty `Name`.
This will make the test more resistant to accidental changes in provider definitions or ConfigMap key names.

Suggested implementation:

```golang
		It("should have valid per-provider YAML files in providers ConfigMap", func() {
			By("Reconciling providers configmap")
			err := reconciler.reconcileProvidersConfigMap(ctx, evalHub)
			Expect(err).NotTo(HaveOccurred())

			By("Getting providers configmap")
			configMap := waitForConfigMap(evalHubName+"-providers", testNamespace)

			By("Ensuring providers ConfigMap has exactly the expected provider keys")
			Expect(configMap.Data).To(HaveLen(len(expectedProviderBenchmarks)))
			for providerKey := range expectedProviderBenchmarks {
				Expect(configMap.Data).To(HaveKey(providerKey))
			}

			By("Validating each provider's schema and benchmarks")
			for providerKey, providerYAML := range configMap.Data {
				expectedBenchmarks, ok := expectedProviderBenchmarks[providerKey]
				Expect(ok).To(BeTrue(), "unexpected provider key %q in providers ConfigMap", providerKey)

				expectedType, ok := expectedProviderTypes[providerKey]
				Expect(ok).To(BeTrue(), "missing expected type for provider key %q", providerKey)

				var provider ProviderResource
				err := yaml.Unmarshal([]byte(providerYAML), &provider)
				Expect(err).NotTo(HaveOccurred(), "provider %q YAML should be valid", providerKey)

				Expect(provider.Name).NotTo(BeEmpty(), "provider %q should have a non-empty name", providerKey)
				Expect(provider.Type).To(Equal(expectedType), "provider %q should have expected type", providerKey)

				actualBenchmarkNames := benchmarkResourceNames(provider)
				Expect(actualBenchmarkNames).To(ConsistOf(expectedBenchmarks), "provider %q benchmarks should match expected set", providerKey)
			}

```

To make this compile and reflect the intended stronger assertions, you will also need to:

1. Define the expected per-provider benchmarks, keyed by the providers ConfigMap keys, near the top of the test file (or in a suitable shared test helper), for example:
   ```go
   var expectedProviderBenchmarks = map[string][]string{
       "aws":      {"benchmark-1", "benchmark-2"},
       "azure":    {"benchmark-1", "benchmark-3"},
       "gcp":      {"benchmark-1", "benchmark-4"},
       "onprem":   {"benchmark-2"},
   }
   ```
   Adjust keys and benchmark names to match your actual provider schema and what `reconcileProvidersConfigMap` is expected to produce.

2. Define the expected provider types, keyed by the same provider keys:
   ```go
   var expectedProviderTypes = map[string]string{
       "aws":    "cloud",
       "azure":  "cloud",
       "gcp":    "cloud",
       "onprem": "datacenter",
   }
   ```
   Again, adjust to align with your real `ProviderResource.Type` values.

3. Ensure `ProviderResource` and `benchmarkResourceNames` are available in this test file:
   - If `ProviderResource` is defined in another package, import that package and reference it as needed.
   - `benchmarkResourceNames` should accept a `ProviderResource` and return `[]string` of benchmark names; if it currently has a different signature, adapt the call accordingly or add a thin helper with that signature.

4. Confirm that the file already imports the `yaml` package used for unmarshalling (e.g. `sigs.k8s.io/yaml`). If not, add the appropriate import consistent with the rest of the file.
</issue_to_address>

### Comment 3
<location> `controllers/evalhub/deployment_test.go:402` </location>
<code_context>

-			By("Checking evalhub container has 2 volume mounts (config + DB secret)")
-			Expect(evalHubContainer.VolumeMounts).To(HaveLen(2))
+			By("Checking evalhub container has 3 volume mounts (config + providers + DB secret)")
+			Expect(evalHubContainer.VolumeMounts).To(HaveLen(3))

</code_context>

<issue_to_address>
**suggestion (testing):** Deployment tests should assert providers volume mount path and ConfigMap name, not only counts and names

Since this PR changes `PROVIDERS_CONFIG_PATH` to `/etc/evalhub/providers/`, the tests should also:
- Verify the `evalhub-providers` VolumeMount has `MountPath == "/etc/evalhub/providers"`.
- Verify the `evalhub-providers` volume is a ConfigMap with `LocalObjectReference.Name == instance.Name + "-providers"`.

This will better protect against regressions in the mount path or ConfigMap wiring.

Suggested implementation:

```golang
			By("Checking evalhub container has 3 volume mounts (config + providers + DB secret)")
			Expect(evalHubContainer.VolumeMounts).To(HaveLen(3))

			By("Checking evalhub-providers VolumeMount path")
			var providersMount *corev1.VolumeMount
			for i := range evalHubContainer.VolumeMounts {
				vm := &evalHubContainer.VolumeMounts[i]
				if vm.Name == "evalhub-providers" {
					providersMount = vm
					break
				}
			}
			Expect(providersMount).NotTo(BeNil())
			Expect(providersMount.MountPath).To(Equal("/etc/evalhub/providers"))

			By("Checking evalhub-providers volume is a ConfigMap with the correct name")
			var providersVolume *corev1.Volume
			for i := range deployment.Spec.Template.Spec.Volumes {
				v := &deployment.Spec.Template.Spec.Volumes[i]
				if v.Name == "evalhub-providers" {
					providersVolume = v
					break
				}
			}
			Expect(providersVolume).NotTo(BeNil())
			Expect(providersVolume.ConfigMap).NotTo(BeNil())
			Expect(providersVolume.ConfigMap.LocalObjectReference.Name).To(Equal(instance.Name + "-providers"))

```

1. Ensure `corev1` is already imported as `k8s.io/api/core/v1`; most likely it is since other volume assertions are present.
2. This snippet assumes an `instance` variable is in scope in this test (e.g. the CR under test). If the CR variable uses a different name, adjust `instance.Name` accordingly.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +132 to +141
It("should have valid per-provider YAML files in providers ConfigMap", func() {
By("Reconciling providers configmap")
err := reconciler.reconcileProvidersConfigMap(ctx, evalHub)
Expect(err).NotTo(HaveOccurred())

By("Getting configmap")
configMap := waitForConfigMap(evalHubName+"-config", testNamespace)
By("Getting providers configmap")
configMap := waitForConfigMap(evalHubName+"-providers", testNamespace)

By("Parsing providers.yaml")
var providersData map[string]interface{}
err = yaml.Unmarshal([]byte(configMap.Data["providers.yaml"]), &providersData)
Expect(err).NotTo(HaveOccurred())

By("Checking providers structure")
Expect(providersData).To(HaveKey("providers"))
providers, ok := providersData["providers"].([]interface{})
Expect(ok).To(BeTrue())
Expect(providers).To(HaveLen(4))
By("Checking each provider file exists and is valid")
expectedProviders := []string{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Strengthen providers ConfigMap test by asserting exact keys and expected provider shapes (type and benchmarks) rather than only non-empty name

Currently the test only checks that four expected keys exist and that each unmarshalled ProviderResource has a non-empty Name. To better match the new provider schema, tighten the assertions by:

  • Verifying configMap.Data contains exactly the expected provider keys (e.g. HaveLen(4) and explicit key set).
  • After unmarshalling, asserting provider.Type and the expected benchmark names per provider (using benchmarkResourceNames), not just a non-empty Name.
    This will make the test more resistant to accidental changes in provider definitions or ConfigMap key names.

Suggested implementation:

		It("should have valid per-provider YAML files in providers ConfigMap", func() {
			By("Reconciling providers configmap")
			err := reconciler.reconcileProvidersConfigMap(ctx, evalHub)
			Expect(err).NotTo(HaveOccurred())

			By("Getting providers configmap")
			configMap := waitForConfigMap(evalHubName+"-providers", testNamespace)

			By("Ensuring providers ConfigMap has exactly the expected provider keys")
			Expect(configMap.Data).To(HaveLen(len(expectedProviderBenchmarks)))
			for providerKey := range expectedProviderBenchmarks {
				Expect(configMap.Data).To(HaveKey(providerKey))
			}

			By("Validating each provider's schema and benchmarks")
			for providerKey, providerYAML := range configMap.Data {
				expectedBenchmarks, ok := expectedProviderBenchmarks[providerKey]
				Expect(ok).To(BeTrue(), "unexpected provider key %q in providers ConfigMap", providerKey)

				expectedType, ok := expectedProviderTypes[providerKey]
				Expect(ok).To(BeTrue(), "missing expected type for provider key %q", providerKey)

				var provider ProviderResource
				err := yaml.Unmarshal([]byte(providerYAML), &provider)
				Expect(err).NotTo(HaveOccurred(), "provider %q YAML should be valid", providerKey)

				Expect(provider.Name).NotTo(BeEmpty(), "provider %q should have a non-empty name", providerKey)
				Expect(provider.Type).To(Equal(expectedType), "provider %q should have expected type", providerKey)

				actualBenchmarkNames := benchmarkResourceNames(provider)
				Expect(actualBenchmarkNames).To(ConsistOf(expectedBenchmarks), "provider %q benchmarks should match expected set", providerKey)
			}

To make this compile and reflect the intended stronger assertions, you will also need to:

  1. Define the expected per-provider benchmarks, keyed by the providers ConfigMap keys, near the top of the test file (or in a suitable shared test helper), for example:

    var expectedProviderBenchmarks = map[string][]string{
        "aws":      {"benchmark-1", "benchmark-2"},
        "azure":    {"benchmark-1", "benchmark-3"},
        "gcp":      {"benchmark-1", "benchmark-4"},
        "onprem":   {"benchmark-2"},
    }

    Adjust keys and benchmark names to match your actual provider schema and what reconcileProvidersConfigMap is expected to produce.

  2. Define the expected provider types, keyed by the same provider keys:

    var expectedProviderTypes = map[string]string{
        "aws":    "cloud",
        "azure":  "cloud",
        "gcp":    "cloud",
        "onprem": "datacenter",
    }

    Again, adjust to align with your real ProviderResource.Type values.

  3. Ensure ProviderResource and benchmarkResourceNames are available in this test file:

    • If ProviderResource is defined in another package, import that package and reference it as needed.
    • benchmarkResourceNames should accept a ProviderResource and return []string of benchmark names; if it currently has a different signature, adapt the call accordingly or add a thin helper with that signature.
  4. Confirm that the file already imports the yaml package used for unmarshalling (e.g. sigs.k8s.io/yaml). If not, add the appropriate import consistent with the rest of the file.


By("Checking evalhub container has 2 volume mounts (config + DB secret)")
Expect(evalHubContainer.VolumeMounts).To(HaveLen(2))
By("Checking evalhub container has 3 volume mounts (config + providers + DB secret)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Deployment tests should assert providers volume mount path and ConfigMap name, not only counts and names

Since this PR changes PROVIDERS_CONFIG_PATH to /etc/evalhub/providers/, the tests should also:

  • Verify the evalhub-providers VolumeMount has MountPath == "/etc/evalhub/providers".
  • Verify the evalhub-providers volume is a ConfigMap with LocalObjectReference.Name == instance.Name + "-providers".

This will better protect against regressions in the mount path or ConfigMap wiring.

Suggested implementation:

			By("Checking evalhub container has 3 volume mounts (config + providers + DB secret)")
			Expect(evalHubContainer.VolumeMounts).To(HaveLen(3))

			By("Checking evalhub-providers VolumeMount path")
			var providersMount *corev1.VolumeMount
			for i := range evalHubContainer.VolumeMounts {
				vm := &evalHubContainer.VolumeMounts[i]
				if vm.Name == "evalhub-providers" {
					providersMount = vm
					break
				}
			}
			Expect(providersMount).NotTo(BeNil())
			Expect(providersMount.MountPath).To(Equal("/etc/evalhub/providers"))

			By("Checking evalhub-providers volume is a ConfigMap with the correct name")
			var providersVolume *corev1.Volume
			for i := range deployment.Spec.Template.Spec.Volumes {
				v := &deployment.Spec.Template.Spec.Volumes[i]
				if v.Name == "evalhub-providers" {
					providersVolume = v
					break
				}
			}
			Expect(providersVolume).NotTo(BeNil())
			Expect(providersVolume.ConfigMap).NotTo(BeNil())
			Expect(providersVolume.ConfigMap.LocalObjectReference.Name).To(Equal(instance.Name + "-providers"))
  1. Ensure corev1 is already imported as k8s.io/api/core/v1; most likely it is since other volume assertions are present.
  2. This snippet assumes an instance variable is in scope in this test (e.g. the CR under test). If the CR variable uses a different name, adjust instance.Name accordingly.

Signed-off-by: tarilabs <matteo.mortari@gmail.com>
@tarilabs tarilabs requested a review from ruivieira February 13, 2026 20:15
@openshift-ci
Copy link

openshift-ci bot commented Feb 13, 2026

@tarilabs: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/trustyai-service-operator-e2e cd42314 link true /test trustyai-service-operator-e2e

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@ruivieira
Copy link
Member

ruivieira commented Feb 15, 2026

@tarilabs thanks for the PR!

I like this, but I'd like to suggest that we have one ConfigMap per provider. OOTB providers can absolutely stay in the Go code, as this would also solve the problem of image injection!

But I think that having separate CMs for each provider makes a smoother path for custom providers. A user who wants to register their own provider would just create a ConfigMap in the namespace and label it with something like evalhub.trustyai.opendatahub.io/provider: "true". The operator would watch for ConfigMaps with that label and mount them as additional volumes on the eval-hub pod, say under /etc/evalhub/providers.d/. Since the eval-hub service already scans directories for provider YAML files, so it would just pick them up.

Adding or removing a custom provider would trigger a normal deployment rollout, which I believe it's the right behaviour.

wdyt?

@tarilabs
Copy link
Member Author

@ruivieira I fully agree the "to be" solution is for the operator to simply mount a number of providers (from CM, from CRD) to the EH Service without knowing the content, but this will require fixing how to deal with Multi-Tenancy, given for example we can't mount CM from Namespace (iff we make the assumption that a Tenant is a Namespace).

In the meantime I just wanted to align closer the resemblance of the ootb providers(this PR), and indeed not added the dependency to the go.mod and vendor'd the struct

It's fine if we want to skip this PR and go straight to MT once "fixed" too :)

Database *DatabaseConfig `yaml:"database,omitempty"`
Secrets *SecretsMapping `yaml:"secrets,omitempty"`
Providers []ProviderResource `yaml:"providers"`
Collections []string `yaml:"collections,omitempty"`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the goal for Collections as strings in this config ?

Enabled: true,
Benchmarks: []string{
"faithfulness", "answer_relevancy", "context_precision", "context_recall",
Providers: []ProviderResource{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want all BYOF providers to be specified in the CR or via separate ConfigMaps that the CR can point to ? If the number of providers grow keeping them in a single CR can be overwhelming leading to large objects. If we keep one ConfigMap per provider, can help keeping things more isolated.

Config: map[string]string{
"bias_threshold": "0.1",
},
Collections: []string{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be sufficient to have collection names pre-defined. We need the actual collection specifications defined. Collection objects are not very small so this could overwhelm the CR.

@tarilabs
Copy link
Member Author

no longer needed, also #649

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants