-
Notifications
You must be signed in to change notification settings - Fork 36
Fixes bug in sandbox_client that assumed pod name matched the sandbox name #150
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
✅ Deploy Preview for agent-sandbox canceled.
|
|
Welcome @igooch! |
|
Hi @igooch. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
af85a8d to
e66fb1d
Compare
aditya-shantanu
left a comment
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.
Add a test in this PR or have a fast follow up.
Do we have the framework set up to be able to test the python client ?
clients/python/agentic-sandbox-client/agentic_sandbox/sandbox_client.py
Outdated
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aditya-shantanu, igooch The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
clients/python/agentic-sandbox-client/agentic_sandbox/sandbox_client.py
Outdated
Show resolved
Hide resolved
| if cond.get('type') == 'Ready' and cond.get('status') == 'True': | ||
| is_ready = True | ||
| break | ||
| if event["type"] in ["ADDED", "MODIFIED"]: |
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.
Is this necessary for the bug fix?
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.
Not for this particular error. It does help with other possible errors as it filters for only the relevant events in the watch.
clients/python/agentic-sandbox-client/agentic_sandbox/sandbox_client.py
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
There is not currently a framework to test the python client. Also, as it stands the So for an e2e test we'll either need to make this client more generic, or have a published version of the |
This pull request addresses a bug in the sandbox_client.py that caused it to fail when using a SandboxWarmPool. The client made incorrect assumptions about the naming of Sandbox and Pod resources when they are sourced from a warm pool, leading to "not found" errors.
Specifically, this pull request introduces the change:
Correct Pod Name for Port-Forwarding: The client now correctly identifies the pod name for port-forwarding by checking for the
agents.x-k8s.io/pod-name annotationon the Sandbox resource. If the annotation is present (indicating the pod is from a sandbox warm pool), it uses the specified pod name. Otherwise, it falls back to using the Sandbox name, preserving the existing behavior for on-demand sandboxes.This change make the Python client fully compatible with SandboxWarmPools.
Other changes are for autopep8 formatting only.
Fixes #149