-
Notifications
You must be signed in to change notification settings - Fork 144
Complete the move of cmd/thv-registry-api outside of toolhive #2244
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
Signed-off-by: Radoslav Dimitrov <[email protected]>
0456d0b to
aa5a05d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2244 +/- ##
==========================================
- Coverage 53.43% 53.17% -0.27%
==========================================
Files 230 221 -9
Lines 29385 28621 -764
==========================================
- Hits 15703 15218 -485
+ Misses 12552 12299 -253
+ Partials 1130 1104 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Radoslav Dimitrov <[email protected]>
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
Completes relocation of the thv-registry-api code out of this repository.
- Removes all source, tests, docs, and build tasks for cmd/thv-registry-api
- Updates image references from ghcr.io/stacklok/toolhive/thv-registry-api to ghcr.io/stacklok/thv-registry-api
- Leaves Helm chart registryAPI values and README entries in place despite removal of all corresponding templates and code
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/charts/operator/values.yaml | Updated registry API image reference (old path to new path). RegistryAPI config still present. |
| deploy/charts/operator/templates/registry-api-serviceaccount.yaml | Removed service account template for registry API. |
| deploy/charts/operator/templates/registry-api-clusterrolebinding.yaml | Removed ClusterRoleBinding for registry API. |
| deploy/charts/operator/templates/registry-api-clusterrole.yaml | Removed ClusterRole for registry API. |
| deploy/charts/operator/README.md | Bumped chart version and updated documented image path; still documents registryAPI values. |
| deploy/charts/operator/Chart.yaml | Chart version bumped from 0.2.23 to 0.2.24. |
| cmd/thv-operator/pkg/registryapi/deployment.go | Updated default image path. |
| cmd/thv-operator/pkg/registryapi/deployment_test.go | Adjusted tests to expect new image path. |
| cmd/thv-operator/Taskfile.yml | Removed local build/load steps for registry API image; updated description. |
| cmd/help/verify.sh | Removed exclude flag now that registry API code is gone. |
| Taskfile.yml | Removed inclusion of registry-api Taskfile and exclude flag in docs generation. |
| (multiple cmd/thv-registry-api/* files) | Entire registry API implementation, tests, docs, and build tasks removed from repo. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dmartinol
left a comment
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.
The operator must ensure the registry RBAC resources are there, we can't remove them.
Also, the image must be in kind to ensure the tests run propertly (even if I believe we're not testing with kind any registry use case)
Signed-off-by: Radoslav Dimitrov <[email protected]>
dmartinol
left a comment
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.
LGTM
jhrozek
left a comment
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.
All review comments have been addressed:
- Threads #3-5: Confirmed that commit 438c810 already implemented the requested changes (kind image loading, RBAC templates restoration, and Taskfile configuration)
- Threads #1-2: Clarified that
registryAPIconfiguration values are actively used by the operator to deploy external registry-api images when MCPRegistry resources are created
The operator follows a dynamic deployment pattern where it creates registry-api Deployment/Service resources programmatically, using the Helm values to configure which external image to deploy and what RBAC permissions to grant.
|
relate to #2103 |
The following PR completes the move of the thv-registry-api server to stacklok/toolhive-registry-server.