-
Notifications
You must be signed in to change notification settings - Fork 564
Add transport type property to Packages #340
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 transport type property to Packages #340
Conversation
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
|
lgtm! for local servers that aren't stdio, can these be easily modelled with the current schema? or what's the general pattern you've seen with these? |
| "sse" | ||
| "streamable-http", | ||
| "sse", | ||
| "stdio" |
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.
@rdimitrov @domdomegg do we actually want stdio on the Remote object? I don't think that is possible, is it? Remote necessarily implies an HTTP connection.
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 guess if we are having it there for consistency of a unified TransportType, maybe it's fine, no real harm in letting it be there... don't feel strongly, just checking I'm not missing some scenario where this is possible
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.
You're right 👍 I think it's better to leave it out of remotes, I'll file a quick PR to fix that in the schema. Thanks for noticing it.
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.
Fix in - #343
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.
Hello, @rdimitrov
The OpenAPI specification for the official registry API, openapi.yaml have transport_type as streamable and sse
Wouldn't changing streamable to streamable-http raise a value error mentioning that values must be one of ('streamable', 'sse') ?
Or are you planning to change the openapi.yaml specification as well ?
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.
Thank you, @kkkarthik! 🙏 I must have missed it while doing the rename, I'll file a PR shortly 🚀
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.
Fix in - #345
I think so, should be fine. The pattern is generally something like the usual stdio command plus a flag like stdio: SSE: |
I think we should have everything necessary to model this 👍 I'm in the process of adopting the ServerJSON format on our side so I'll be able to verify and test this. I'll let you know if I happen to find any issues 👍 |
I guess my concern would be around something like the server runs locally on sse/shttp, but then how do you know what URl to connect at? I guess you could also set |
I think it's not necessary to be advertised via remotes, usually it's on |
…343) <!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> Addresses #340 (comment) ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> ## Breaking Changes <!-- Will users need to update their code or configurations? --> No ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
…#343) <!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> Addresses modelcontextprotocol/registry#340 (comment) ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> ## Breaking Changes <!-- Will users need to update their code or configurations? --> No ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Motivation and Context
The following PR:
How Has This Been Tested?
Locally
Breaking Changes
No
Types of changes
Checklist
Additional context
This was discussed in #200