-
Notifications
You must be signed in to change notification settings - Fork 5
Add the mcp connection plugin #9
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
Add the mcp connection plugin #9
Conversation
4215cf0 to
42fec2e
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 54s |
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 51s |
6e38155 to
3cac395
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 6m 58s |
ea24dbb to
b4cbf12
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 43s |
b4cbf12 to
af69698
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 43s |
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 51s |
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 will be worth adding unit tests for the single method _load_server_from_manifest() and _create_transport()
| mkdir | ||
| ln | ||
| rm | ||
| ansible-galaxy |
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 should be added as depency
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.
If I do not include ansible_galaxy here, it will complain:
linters: failed with ansible-galaxy is not allowed, use allowlist_externals to allow it linters: FAIL code 1 (6.99 seconds)
| def close(self) -> None: | ||
| """Terminate the persistent connection and reset state.""" | ||
| display.vvv("[mcp] Closing MCP connection") | ||
|
|
||
| self._close_client() | ||
| super().close() # sets _conn_closed, _connected | ||
|
|
||
| def _close_client(self) -> None: | ||
| """Close the MCPClient if it exists and reset the reference.""" | ||
| if not self._client: | ||
| display.vvv("[mcp] No MCP client to close") | ||
| return | ||
|
|
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.
why not merging the 2 methods into a single one?
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.
_close_client() exclusively handle the dependency-specific shutdown and error handling for the MCPClient, while close() is a high-level method responsible for coordinating the overall cleanup and calling super().close() for base class state management.
plugins/connection/mcp.py
Outdated
| type: dict | ||
| vars: | ||
| - name: ansible_mcp_mcp_server_env | ||
| mcp_bearer_token: |
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.
Can we also read this from an MCP_BEARER_TOKEN env var?
plugins/connection/mcp.py
Outdated
|
|
||
| return manifest[server_name] | ||
|
|
||
| def _create_transport(self, server_name: str, server_info: dict): |
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.
The type hinting should show a return type of Transport.
73abd34 to
e11669a
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 55s |
plugins/connection/mcp.py
Outdated
| - Both stdio and Streamable HTTP transport methods are supported. | ||
| - All tasks using this connection plugin are run on the Ansible control node. | ||
| options: | ||
| mcp_server_name: |
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.
Sorry, just now noticing. We should probably remove the mcp_ prefix from these options so we don't end up with ansible_mcp_mcp_ for config vars.
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.
No problem, makes sense, removed!
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
e11669a to
82a448c
Compare
|
Build succeeded. ✔️ build-ansible-collection SUCCESS in 5m 48s |
SUMMARY
Add the mcp connection plugin
Depends On #7
ISSUE TYPE
COMPONENT NAME
mcp