Conversation
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ToolHive Registry Server documentation for version 0.6.0, introducing several new features and clarifications around deployment, database configuration, and workload discovery.
Changes:
- Added namespace-scoped workload discovery configuration with RBAC examples for both cluster-wide and namespace-scoped deployments
- Updated database documentation to reflect that PostgreSQL is now required (not optional) and added AWS RDS IAM authentication support
- Added new configuration fields including
maxMetaSize,--auth-modeflag, Git authentication options, and thetoolhive.stacklok.dev/toolsannotation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/toolhive/guides-registry/deployment.mdx | Removed in-memory store warning, added comprehensive namespace discovery configuration with RBAC examples for cluster-wide and namespace-scoped deployments, and documented leader election ID configuration |
| docs/toolhive/guides-registry/database.mdx | Changed database from optional to required, added maxMetaSize configuration field, and documented AWS RDS IAM dynamic authentication |
| docs/toolhive/guides-registry/configuration.mdx | Updated database comments from optional to required, added --auth-mode flag, documented Git authentication fields, added toolhive.stacklok.dev/tools annotation, and updated references to align with deployment documentation |
Member
Author
|
The output of running the Now I have everything I need for a thorough editorial review. Let me compile my findings.
Summary
The changes are well-executed and accurately reflect the v0.6.0 release. The cross-document deduplication (THV_REGISTRY_WATCH_NAMESPACE) is handled cleanly. The main issues
are minor: a couple of wording inconsistencies in new content and one structural concern in the deployment.mdx RBAC section.
Primary Issues
1. Workload Discovery section heading casing (deployment.mdx:180)
The heading ## Workload Discovery uses title case, but the project style guide requires sentence case for headings. The other H2s on this page use sentence case ("Kubernetes
deployment").
Ask: Change to ## Workload discovery.
2. "Workload Discovery" paragraph repeats what the subsection headings already say (deployment.mdx:202-204)
The intro paragraph:
Both RBAC options below use the same ClusterRole for workload discovery and a separate namespace-scoped Role for leader election. The difference is how the ClusterRole is
bound.
This is useful framing. However, the subsection heading at line 208 then immediately restates:
When THV_REGISTRY_WATCH_NAMESPACE is not set, the server watches all namespaces and requires a ClusterRole and ClusterRoleBinding
This re-explains what the reader just learned two lines above. Consider trimming the subsection opening to just jump into the YAML.
Ask: Shorten line 208-209 to just a brief lead-in, e.g., "Apply the following resources for cluster-wide discovery:" — since the intro paragraph already explained the pattern.
3. Dynamic authentication section intro is slightly redundant with the H2 (database.mdx:127-129)
Lines 127-129:
As an alternative to pgpass files, you can use dynamic authentication to generate database credentials at runtime. This is useful for cloud-hosted databases that support
IAM-based authentication.
Then lines 133-135:
When running on AWS, you can authenticate to RDS using IAM credentials instead of static passwords. The server generates short-lived authentication tokens using the IAM role
attached to the workload.
Both paragraphs say "use IAM instead of static credentials." Since there's currently only one provider (AWS RDS), the section intro could be shorter — the AWS subsection
already explains the concept.
Ask: Tighten the H2 intro to one sentence, e.g.: "As an alternative to pgpass files, the server supports dynamic credential generation for cloud-hosted databases."
Secondary Issues
Issue: "the Registry Server" vs "the server" — inconsistent capitalization in new content
Location: configuration.mdx:234, deployment.mdx:188 vs deployment.mdx:199
Recommendation: Pick one and use consistently within each page. The existing pages favor "the server" (lowercase) in prose and "Registry Server" only for the product name on
first reference.
────────────────────────────────────────
Issue: file and url mutual exclusivity description is confusing (pre-existing)
Location: configuration.mdx:179
Recommendation: "The fields file and url are mutually exclusive" — but file is the parent, and url is nested under it. This is a pre-existing issue, not introduced by the PR,
so skip for this review.
────────────────────────────────────────
Issue: The cross-reference link text "Workload Discovery" uses title case
Location: configuration.mdx:237
Recommendation: Should match the heading it links to. If the heading changes to sentence case, update the link text too.
Inline Suggestions
1. deployment.mdx:180 — ## Workload Discovery → ## Workload discovery (sentence case per style guide)
2. deployment.mdx:208-209 — Consider shortening:
- When `THV_REGISTRY_WATCH_NAMESPACE` is not set, the server watches all
- namespaces and requires a ClusterRole and ClusterRoleBinding:
+ For cluster-wide discovery, apply the following resources:
3. database.mdx:127-129 — Consider shortening:
- As an alternative to pgpass files, you can use dynamic authentication to
- generate database credentials at runtime. This is useful for cloud-hosted
- databases that support IAM-based authentication.
+ As an alternative to pgpass files, the server supports dynamic credential
+ generation for cloud-hosted databases.
4. configuration.mdx:237 — Update link text if heading changes: [Workload discovery] (lowercase "d") |
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Member
Author
|
Result of asking Claude to address the findings (addressed in 2d2761b: All checks pass. Here's a summary of all four review items addressed:
1. Heading casing: ## Workload Discovery → ## Workload discovery (deployment.mdx) + matching link text updated (configuration.mdx)
2. Redundant subsection opener: Replaced "When THV_REGISTRY_WATCH_NAMESPACE is not set, the server watches all namespaces and requires..." with "For cluster-wide discovery,
apply the following resources:" (deployment.mdx)
3. Verbose dynamic auth intro: Three lines → two, removing overlap with the AWS subsection (database.mdx)
4. Inconsistent capitalization: "Registry Server" → "Registry server" in two places to match existing convention (configuration.mdx, deployment.mdx) |
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
danbarr
reviewed
Feb 12, 2026
|
|
||
| For cluster-wide discovery, apply the following resources: | ||
|
|
||
| {/* prettier-ignore */} |
Collaborator
There was a problem hiding this comment.
Was prettier breaking some formatting of the YAML block? (I've been on the hunt for examples of this to see if we can make any config changes or if it's just limitations in Prettier itself)
danbarr
approved these changes
Feb 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary of the changes:
Type of change
Related issues/PRs
Screenshots
Submitter checklist
Content and formatting
Navigation
sidebars.ts) updated for added, deleted, reordered, or renamed filesvercel.jsonfor moved, renamed, or deleted pages (i.e., if the URL slug changed)Reviewer checklist
Content