feat: Add SERVICE_URL environment variable to operator deployment#632
feat: Add SERVICE_URL environment variable to operator deployment#632ruivieira wants to merge 1 commit intotrustyai-explainability:mainfrom
Conversation
Signed-off-by: Rui Vieira <ruidevieira@googlemail.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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds a new SERVICE_URL environment variable to the EvalHub operator deployment and covers it with a unit test to ensure the URL is constructed correctly from the EvalHub name and namespace. Flow diagram for constructing SERVICE_URL environment variableflowchart TD
A[EvalHubReconciler.buildDeploymentSpec<br/>inputs: ctx, instance EvalHub] --> B[Read instance.Name]
A --> C[Read instance.Namespace]
B --> D[Format SERVICE_URL<br/>pattern: http://%s.%s.svc.cluster.local:8080]
C --> D
D --> E[Create EnvVar SERVICE_URL<br/>value: formatted URL]
E --> F[Append SERVICE_URL to container env list]
F --> G[Return Deployment spec<br/>with SERVICE_URL set]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughA new default environment variable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
SERVICE_URLvalue hardcodes both the port (8080) and assumes the Service name matchesinstance.Name; if either of these can differ from the deployment name, consider deriving them from the Service spec or shared constants instead of formatting directly frominstance. - Embedding the namespace directly in the URL string ties this env var to in-cluster DNS; if this URL is intended to be consumed outside the cluster or across environments, you may want to make the scheme/host configurable via the CR or operator configuration rather than fully constructing it here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `SERVICE_URL` value hardcodes both the port (`8080`) and assumes the Service name matches `instance.Name`; if either of these can differ from the deployment name, consider deriving them from the Service spec or shared constants instead of formatting directly from `instance`.
- Embedding the namespace directly in the URL string ties this env var to in-cluster DNS; if this URL is intended to be consumed outside the cluster or across environments, you may want to make the scheme/host configurable via the CR or operator configuration rather than fully constructing it here.
## Individual Comments
### Comment 1
<location> `controllers/evalhub/deployment.go:111` </location>
<code_context>
+ {
+ Name: "SERVICE_URL",
+ Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", instance.Name, instance.Namespace),
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the cluster DNS suffix to support non-default cluster domains.
Hardcoding `svc.cluster.local` assumes the default cluster DNS suffix and will fail on clusters with a custom domain (e.g. `svc.cluster.mycorp`). Please make the cluster domain configurable (e.g. flag/env) or derive it from existing operator configuration and compose the URL from that.
Suggested implementation:
```golang
{
Name: "SERVICE_URL",
Value: fmt.Sprintf("http://%s.%s.svc.%s:8080", instance.Name, instance.Namespace, getClusterDomain()),
},
```
To fully support a configurable cluster domain, you also need to:
1. **Add an import for `os`** at the top of `controllers/evalhub/deployment.go` (if it is not already there):
```go
import (
// existing imports...
"os"
)
```
2. **Define the `getClusterDomain()` helper** in `controllers/evalhub/deployment.go` (place it near other helper functions):
```go
func getClusterDomain() string {
if v := os.Getenv("CLUSTER_DOMAIN"); v != "" {
return v
}
// Fallback to the default Kubernetes cluster domain
return "cluster.local"
}
```
3. Optionally, if your operator already has a configuration or flag system, you may want to:
- Plumb the cluster domain from that configuration instead of directly reading `os.Getenv` in this file.
- In that case, adjust the `getClusterDomain()` implementation to read from your existing config object rather than from the environment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -109,6 +109,10 @@ func (r *EvalHubReconciler) buildDeploymentSpec(ctx context.Context, instance *e | |||
| Name: "PROVIDERS_CONFIG_PATH", | |||
| Value: "/etc/evalhub/providers.yaml", | |||
| }, | |||
There was a problem hiding this comment.
suggestion: Avoid hardcoding the cluster DNS suffix to support non-default cluster domains.
Hardcoding svc.cluster.local assumes the default cluster DNS suffix and will fail on clusters with a custom domain (e.g. svc.cluster.mycorp). Please make the cluster domain configurable (e.g. flag/env) or derive it from existing operator configuration and compose the URL from that.
Suggested implementation:
{
Name: "SERVICE_URL",
Value: fmt.Sprintf("http://%s.%s.svc.%s:8080", instance.Name, instance.Namespace, getClusterDomain()),
},To fully support a configurable cluster domain, you also need to:
-
Add an import for
osat the top ofcontrollers/evalhub/deployment.go(if it is not already there):import ( // existing imports... "os" )
-
Define the
getClusterDomain()helper incontrollers/evalhub/deployment.go(place it near other helper functions):func getClusterDomain() string { if v := os.Getenv("CLUSTER_DOMAIN"); v != "" { return v } // Fallback to the default Kubernetes cluster domain return "cluster.local" }
-
Optionally, if your operator already has a configuration or flag system, you may want to:
- Plumb the cluster domain from that configuration instead of directly reading
os.Getenvin this file. - In that case, adjust the
getClusterDomain()implementation to read from your existing config object rather than from the environment.
- Plumb the cluster domain from that configuration instead of directly reading
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controllers/evalhub/deployment.go`:
- Around line 112-115: Replace the hardcoded "8080" with the shared
containerPort constant: change the SERVICE_URL env var construction (the
fmt.Sprintf call that uses instance.Name and instance.Namespace) to format using
the containerPort constant from constants.go, and set the API_PORT env var value
to the string form of that same containerPort constant (convert the constant to
a string rather than hardcoding "8080") so both SERVICE_URL and API_PORT derive
from containerPort.
| { | ||
| Name: "SERVICE_URL", | ||
| Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", instance.Name, instance.Namespace), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the specific file and context around lines 112-115
echo "=== Context around deployment.go lines 112-115 ==="
sed -n '100,130p' controllers/evalhub/deployment.go
echo -e "\n=== Search for Service creation/definitions ==="
rg -n --type=go 'corev1\.Service|Kind.*Service' -C 3
echo -e "\n=== Search for port-related constants and configurations ==="
rg -n --type=go 'containerPort|API_PORT|ServicePort|8080' -B 2 -A 2
echo -e "\n=== Check entire deployment.go for port references ==="
rg -n --type=go ':8080|8080' controllers/evalhub/deployment.go -C 2Repository: trustyai-explainability/trustyai-service-operator
Length of output: 50405
🏁 Script executed:
#!/bin/bash
# Find containerPort definition in deployment.go
echo "=== Full deployment.go file to find containerPort definition ==="
cat -n controllers/evalhub/deployment.go | head -150
echo -e "\n=== Search for containerPort variable initialization ==="
rg -n 'containerPort\s*:?=' controllers/evalhub/deployment.go -B 2 -A 2
echo -e "\n=== Check if there's a constants file for EvalHub ==="
fd -e go . controllers/evalhub/ | grep -i const
echo -e "\n=== Look for any port constants in evalhub ==="
rg -n 'const.*[Pp]ort|8080' controllers/evalhub/deployment.go -B 1 -A 1Repository: trustyai-explainability/trustyai-service-operator
Length of output: 5889
🏁 Script executed:
#!/bin/bash
# Check the constants.go file in evalhub
echo "=== evalhub/constants.go ==="
cat -n controllers/evalhub/constants.go
echo -e "\n=== Search for containerPort, API_PORT definitions across entire evalhub ==="
rg -n 'containerPort|apiPort|const.*Port|8080' controllers/evalhub/ --type=go | head -50Repository: trustyai-explainability/trustyai-service-operator
Length of output: 4071
Derive SERVICE_URL and API_PORT from the port constant.
The SERVICE_URL hardcodes port 8080 in the fmt.Sprintf, and API_PORT hardcodes it as a string. A containerPort constant already exists in constants.go (line 17). To keep these in sync and avoid drift if the port changes, use the constant instead.
🔧 Suggested fix
{
Name: "API_PORT",
- Value: "8080",
+ Value: fmt.Sprintf("%d", containerPort),
}, {
Name: "SERVICE_URL",
- Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", instance.Name, instance.Namespace),
+ Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", instance.Name, instance.Namespace, containerPort),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| Name: "SERVICE_URL", | |
| Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", instance.Name, instance.Namespace), | |
| }, | |
| { | |
| Name: "SERVICE_URL", | |
| Value: fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", instance.Name, instance.Namespace, containerPort), | |
| }, |
🤖 Prompt for AI Agents
In `@controllers/evalhub/deployment.go` around lines 112 - 115, Replace the
hardcoded "8080" with the shared containerPort constant: change the SERVICE_URL
env var construction (the fmt.Sprintf call that uses instance.Name and
instance.Namespace) to format using the containerPort constant from
constants.go, and set the API_PORT env var value to the string form of that same
containerPort constant (convert the constant to a string rather than hardcoding
"8080") so both SERVICE_URL and API_PORT derive from containerPort.
|
PR needs rebase. 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. |
1 participant
Refer to RHOAIENG-48376.
Summary by Sourcery
Add a SERVICE_URL environment variable to the EvalHub operator deployment and cover it with tests.
New Features:
Tests:
Summary by CodeRabbit