-
Notifications
You must be signed in to change notification settings - Fork 126
Add an enforce
flag to MCPRegistry
#1997
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
Conversation
d5cf1d3
to
47fbcb5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1997 +/- ##
==========================================
+ Coverage 47.76% 47.90% +0.14%
==========================================
Files 232 233 +1
Lines 29007 29170 +163
==========================================
+ Hits 13856 13975 +119
- Misses 14126 14166 +40
- Partials 1025 1029 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 for submitting these changes!
some minor comments that would not prevent the approval, and a couple of general comments that I'd like to address before.
- What happens if the MCPServer is annotated to match a given registry? Shouldn't we restrict the validation to run only againts that registry?
- Generally speaking, the term
ImageValidation
seems to minimize the feature, because we could extend it to validate the annotations (see ^) and also verify that the associated server exists, is in Active status and also secure and so on (we could add more annotations for that).
Apart from these nits, that can be addressed later, looks good!
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
47fbcb5
to
8814705
Compare
Thank you for the review @dmartinol I implemented the suggestions as individual patches. I must say that because this week is a "conference" week for us I haven't had that much time testing it - I'll do that in a couple of hours in the meantime I wanted the CI to have a go. |
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
8814705
to
deed7fb
Compare
deed7fb
to
c6c0efb
Compare
Extends the MCPRegistry CRD with a new
enforce
flag. This flag defaults to false, but when set to true, the MCP server controller will only reconcile serves that are present in one of the enforcing registries.Fixes: #1976