-
Notifications
You must be signed in to change notification settings - Fork 22
Add store-db MCP server with lazy database connection management #51
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
| if [ -f "$chart/helm/Chart.yaml" ]; then | ||
| helm package "$chart/helm" -d gh-pages | ||
| fi | ||
| done |
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.
@skattoju do we still need this? we don't have subcharts under mcp-servers
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 is not exactly a sub chart but a chart by itself. This is to pick up mcp-servers that are not included in the mcp-servers helm chart. The reason for not including it was because it has additional components other than service and deployment like secrets, configmaps and pvcs etc.
I figured it would be simpler to add it as a standalone chart instead of adding templates to the mcp-server helm chart but it would mean that it would have to be manually installed or added as an additional dependency to a project that wants to include this "non standard" mcp server.
We could add some logic to handle additional templates as well but it would require some re-testing compared to just referring to the known working helm chart. I am curious to know the case against just adding new mcp-server charts as dependencies to the main mcp-server chart ? Wouldn't it effectively be the same where consumers can enable or disable charts they need in the values file ?
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.
I see what you're saying - so you're not deploying an mcp server only, you're deploying a full blown app. It relates to my other question about deploying postgres by default.
The mcp-servers chart deploys mcp servers only, it should be simple like you said, an image with a service etc. I think we need to break this to 2 things - one is an mcp server that will be deployed through this chart and another is the app that the mcp server interacts with. If this is an AI building block, it can be in the toplevel directory of the project, and the mcp server can be under mcp-servers, something like that, wdyt? @skattoju @sauagarwa
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.
Yes the MCP server is kind of an app in its current state. The requirement was to have an example MCP server that interacts with a database. In its current state it will not run without postgres.
There is also an "API version" where the MCP server and the app are separate. I could repackage using that. However, it seems a bit awkward to include the app in the architecture charts repository since it's not exactly AI related.
May be we can include the app portion in AI virtual assistant ?
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.
@yuvalturg I have factored out the mcp part and left the rest to be in ai virtual assistant till we come up with a dedicated place for mcp server if that is something we want to do
|
|
||
| # PostgreSQL Database Configuration | ||
| postgresql: | ||
| enabled: true |
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.
we deploy postgres by default?
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.
Yeah the MCP server is supposed to interact with a database to fetch store specific data albeit via a data layer instead of running queries directly.
| @@ -0,0 +1,131 @@ | |||
| {{- if .Values.postgresql.enabled }} | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
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.
We can probably make this a StatefulSet
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.
Its only one pod so its does not add value for a demo scenario.. but we can add it in case someone wants to have a multiple pods.
- Replace store-inventory with store-db MCP server - Implement lazy database connection management - Remove PostgreSQL dependency from helm chart - Configure external database connection (defaults to pgvector) - Update naming from mcp-dbstore to mcp-store-db - Remove service account creation (uses cluster default) - Integrate into main mcp-servers helm chart - Update GitHub Actions workflow for CI/CD - Add comprehensive documentation and testing
Add Store-DB MCP Server
This PR introduces a new MCP server that provides AI agents with access to a store database by connecting it's tools to it via lazy database connection management so that it can be pointed to a database containing the store database as well as not crash when the database is unavailable.
Key Features
Architecture
MCP Tools
Deployment
Database Setup Required
Create database in your PostgreSQL instance before deployment.
This provides a robust, production-ready MCP server for AI agents needing database access.