Adding tool to fetch the public IP for databases, plus some small fixes in descriptions#107
Adding tool to fetch the public IP for databases, plus some small fixes in descriptions#107shyam2511 wants to merge 1 commit intooracle:mainfrom
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
shyam2511
force-pushed
the
database-mcp-changes
branch
2 times, most recently
from
030c3ed to
6b3fcd0
Compare
| freeform_tags: Optional[str] = Field( | ||
| freeform_tags: Optional[dict] = Field( | ||
| None, | ||
| description='Free-form tags for this resource. Each tag is a simple key-value pair with no predefined name, type, or namespace. For more information, see `Resource Tags`__. Example: `{"Department": "Finance"}` __ https://docs.cloud.oracle.com/Content/General/Concepts/resourcetags.htm', |
There was a problem hiding this comment.
Any examples should be in the examples argument. See https://docs.pydantic.dev/2.0/api/fields/#pydantic.fields.Field
There was a problem hiding this comment.
This model is a pojo that is never visible to the agent, hence the description shouldn't matter here
| return map_pluggabledatabase(response.data) | ||
|
|
||
|
|
||
| def get_public_ip_for_db(database_id, region=None): |
There was a problem hiding this comment.
Can we add unit tests for these changes
|
|
||
| try: | ||
| # Fetch VNIC details | ||
| get_vnic_response = virtual_network_client.get_vnic(node.vnic_id) |
There was a problem hiding this comment.
We already have a tool in the networking mcp server called get_vnic. Is it worth it to just use that instead?
There was a problem hiding this comment.
We require this tool to be in our server itself, that's why we have added it. Otherwise the user will have to install both servers, and the agent would have no way to know how to actually get the public IPs even when given these tools
shyam2511
force-pushed
the
database-mcp-changes
branch
from
6b3fcd0 to
2a33f60
Compare
…es in descriptions.
shyam2511
force-pushed
the
database-mcp-changes
branch
from
2a33f60 to
1a7c01c
Compare
shyam2511
requested review from
AlaaShaker,
gebhardtr and
krisrice
as code owners
shyam2511
changed the title
| @@ -312,11 +312,107 @@ def call_create_pdb(client, details, opc_retry_token=None, opc_request_id=None): | |||
| return map_pluggabledatabase(response.data) | |||
|
|
|||
|
|
|||
| @mcp.tool( | |||
| description="Fetches the public IP address for a specified Database. For a Pluggable Database, use it's container database id as input" | |||
There was a problem hiding this comment.
nit:
| description="Fetches the public IP address for a specified Database. For a Pluggable Database, use it's container database id as input" | |
| description="Fetches the public IP address for a specified Database. For a Pluggable Database, use its container database id as input" |
| token = f.read() | ||
| signer = oci.auth.signers.SecurityTokenSigner(token, private_key) | ||
|
|
||
| virtual_network_client = oci.core.VirtualNetworkClient(config, signer=signer) |
There was a problem hiding this comment.
An advantage of doing it this ways it that you save tokens (and an LLM roundtrip), but this separation of concerns violation complicates your server and will require additional updates when we add support for Oauth and other authentication approaches.
Have you considered changing this tool to return the DB nodes (with VNIC attribute) and allow the model to invoke the correct tool (in the networking server) to find the public IP for the VNIC?
4 participants
Description
Adding tool to fetch the public IP for databases, plus some small fixes in descriptions.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: