-
Notifications
You must be signed in to change notification settings - Fork 46
feature: add agent-sandbox kubernetes provider #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 667ff222db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for pod in pods: | ||
| if pod.status and pod.status.pod_ip and pod.status.phase == "Running": | ||
| return f"{pod.status.pod_ip}:{port}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer serviceFQDN when ingress is expected
The endpoint resolver returns the first Running pod IP before considering status.serviceFQDN. In environments where the API server or clients are outside the cluster (or rely on ingress), pod IPs are not routable, so this returns an unusable endpoint even when a serviceFQDN is available. This makes get_endpoint fail for agent-sandbox deployments that expect ingress/service DNS. Consider preferring the serviceFQDN when present (or when ingress is enabled) to avoid returning unreachable pod IPs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supported now. Implement soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template loader looks like a copy of batchsandbox_template—can we find a way to reuse this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored
Summary
Testing