chore(evalhub): hardcoded providers not needed#649
chore(evalhub): hardcoded providers not needed#649tarilabs wants to merge 1 commit intotrustyai-explainability:mainfrom
Conversation
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughEvalHub controller configuration is simplified by removing the ProviderConfig type, eliminating provider and collection fields from EvalHubConfig, and stopping generation of providers.yaml. Database and secrets configuration remains. RBAC resources are reorganized in kustomization.yaml with adjusted ordering and substituted entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/evalhub/deployment.go (1)
110-114: Consider removing the commented-out code entirely.Commented-out code tends to become stale noise. The TODO intent can be preserved as a standalone comment or tracked in an issue, without leaving the dead
EnvVarstruct literal in the source.Suggested simplification
- // TODO: reconsider when the Operator reads OOTB providers from ConfigMaps - // { - // Name: "PROVIDERS_CONFIG_PATH", - // Value: "/etc/evalhub/providers.yaml", - // }, + // TODO: reconsider reading OOTB providers from ConfigMaps (may reintroduce PROVIDERS_CONFIG_PATH)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/evalhub/deployment.go` around lines 110 - 114, Remove the dead commented EnvVar struct literal that sets PROVIDERS_CONFIG_PATH in deployment.go (the three-line commented block containing Name: "PROVIDERS_CONFIG_PATH", Value: "/etc/evalhub/providers.yaml") and either leave a short TODO comment or create an issue to track the operator ConfigMap behavior; ensure no leftover commented EnvVar entries remain in the deployment spec so the code stays clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/evalhub/deployment.go`:
- Around line 110-114: Remove the dead commented EnvVar struct literal that sets
PROVIDERS_CONFIG_PATH in deployment.go (the three-line commented block
containing Name: "PROVIDERS_CONFIG_PATH", Value: "/etc/evalhub/providers.yaml")
and either leave a short TODO comment or create an issue to track the operator
ConfigMap behavior; ensure no leftover commented EnvVar entries remain in the
deployment spec so the code stays clean.
|
@tarilabs: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
to be closed in favour of #650 |
Summary by CodeRabbit