feat: Support YAML for MCP server configuration#85
feat: Support YAML for MCP server configuration#85zyfy29 wants to merge 4 commits intomcpjungle:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds YAML support for MCP server configuration files, allowing users to register servers using either JSON or YAML format. This enhances user experience by providing flexibility in configuration file formats.
- Added YAML struct tags to RegisterServerInput fields alongside existing JSON tags
- Implemented file extension-based format detection (.json, .yaml, .yml)
- Created separate parsing functions for JSON and YAML formats with comprehensive test coverage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/types/mcp_server.go | Added YAML struct tags to all RegisterServerInput fields |
| cmd/register.go | Refactored config reading to support both JSON and YAML formats with extension detection |
| cmd/register_test.go | Added comprehensive tests for both JSON and YAML configuration parsing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cmd/register.go
Outdated
| } | ||
| defer f.Close() | ||
| ext := filepath.Ext(filePath) | ||
| switch ext { |
There was a problem hiding this comment.
At this point, do you think it is better to take out the whole configuration parsing logic into a separate package?
There are also some other commands that now rely on finding & parsing configurations.
There was a problem hiding this comment.
Yes, this logic should be reuseble.
|
@zyfy29 everything looks great. Mainly 2 things from me:
|
duaraghav8
force-pushed
the
main
branch
2 times, most recently
from
c445a3e to
bf87f96
Compare
|
OK, I'll look at this today. |
# Conflicts: # cmd/register.go
# Conflicts: # pkg/types/mcp_server.go
|
Sorry for the late response, I was preparing for interviews... What do you think of the implementation now? |
|
@zyfy29 thanks! Looks cleaner now, I will do one final round of testing myself and then merge |
duaraghav8
force-pushed
the
main
branch
2 times, most recently
from
e6f9f3d to
321f552
Compare
close #78