Add flags for publish subcommand #771
Conversation
|
@domdomegg @rdimitrov I am unable to add reviewers to this PR (and wasn't sure who should I request for reviews). Please take a look, thank you!! :) |
## Description Temporarily move `server.json` out from the `.registry/` folder. The mcp-publisher library do not support the `--file` flag yet. A [PR](modelcontextprotocol/registry#771) was submitted to add flags for publish subcommand. ## PR Checklist > Thank you for opening a Pull Request! Before submitting your PR, there are a > few things you can do to make sure it goes smoothly: - [x] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/genai-toolbox/blob/main/CONTRIBUTING.md) - [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/genai-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary) - [x] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #<issue_number_goes_here>
| } | ||
|
|
||
| token := tokenInfo["token"] | ||
| registryURL := tokenInfo["registry"] |
There was a problem hiding this comment.
I think the registryUrl probably won't be helpful as your token determines which registry you have authed to
|
|
||
| func PublishCommand(args []string) error { | ||
| publishFlags := flag.NewFlagSet("publish", flag.ExitOnError) | ||
| serverFile := publishFlags.String("file", "server.json", "Path to server.json (default: ./server.json)") |
There was a problem hiding this comment.
My general philosophy is to avoid configuration options as much as possible - each additional flag is another lever of complexity to maintain, debug, document, etc. And they compose exponentially so you increase the surface area for problems a lot.
I can see this being helpful, although I'd ask first why:
- the default isn't working, e.g. you can't name it server.json. Note that you could put this in a subfolder and run publish from there
- failing that, whether it couldn't compose already with other tools (e.g. move the server.json into the right place or a tmp folder, then run publish, then clean up).
There was a problem hiding this comment.
@domdomegg The --file flag is a requirement for our planned expansion to support multiple servers within the registry. These servers will consist of (1) multiple prebuilt tools for specific database connections (typically via stdio) -- each being one server and (2) custom tools that users can build using their own configuration files.
Since the startup args differ between these server types, we will be using different server.json for each server. Currently, we only have the custom tools server published, and planning to expand it next.
|
Also I appreciate these flags are bizarrely in the docs, which seems like a mistake for something not implemented, so we do need to fix something (either the docs or the implementation). I'd lean towards removing the options from the docs. cc @rdimitrov I wonder what you think here? |
2 participants
Add flags for publish subcommand.
Motivation and Context
We'll need to use the
--fileflag since we store our server.json in a folder.The options that was noted in the docs here was not implemented. This PR added the 3 flags that was stated.
How Has This Been Tested?
Tested locally
Breaking Changes
No breaking changes. The existing way of defining file
mcp-publisher publish server.jsonwill still work.Types of changes
Checklist
Additional context
Fixes #767