terrymanu commented on PR #39025: URL: https://github.com/apache/shardingsphere/pull/39025#issuecomment-4900562582
### Summary **Review Result: Mergeable** Reason: Code-scope review only. The PR consistently replaces template-style MCP documentation examples with concrete sample values or runtime-provided endpoint variables, preserves the existing secret-placeholder contract, and keeps the YAML placeholder rejection test aligned with the same validation path. CI not reviewed by request. ### Evidence - The changed ShardingSphere-MCP docs replace old copyable template placeholders such as `<logic-database>`, `<proxy-host>`, `<table-name>`, `https://example.com/mcp`, and `YOUR_TOKEN` with concrete examples or runtime-provided values. - The sensitive placeholder guidance remains intact: `secret_ref`, `placeholder://...`, `secret_reference:*`, and manual `<SECRET_VALUE_...>` placeholders are still documented and backed by existing MCP workflow code and descriptors. - The test-only change in `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/swapper/YamlMCPLaunchConfigurationSwapperTest.java` only renames negative fixture inputs to `<unresolved-*>`; the production rejection path remains owned by `YamlRuntimeDatabaseConfigurationsValidator`. - A focused unit test run on PR head passed: `./mvnw -pl mcp/bootstrap -am -DskipITs -Dspotless.skip=true -Dtest=YamlMCPLaunchConfigurationSwapperTest -Dsurefire.failIfNoSpecifiedTests=false test -B -ntp`, exit 0, `Tests run: 22, Failures: 0, Errors: 0, Skipped: 0`. - Static placeholder scan over the MCP docs, `mcp`, `test/e2e/mcp`, `distribution/mcp`, and `.github/workflows/e2e-mcp.yml` found no residual old template tokens targeted by this PR. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: PR #39025, latest head `8cfe2825651ea8e8f84f22cac3853089ed3ce3e0`, base `master` at `c39dc442faa38aeae92f38886055859d6fbb6b26`, merge-base `c39dc442faa38aeae92f38886055859d6fbb6b26`. Reviewed all 37 changed files listed by GitHub; the local triple-dot file list matched GitHub `/pulls/39025/files`. - Not Reviewed Scope: GitHub Actions/check-runs/logs, rendered website output, and live OpenAI/Anthropic remote MCP platform execution. - Verification: Ran the focused Maven test command above with exit 0. Also ran static scans for the old MCP template placeholders and fake remote endpoint/token examples; no targeted residuals were found. - Release Note / User Docs: User docs are the PR scope and were reviewed. No release note is required because this is documentation/example cleanup plus a test fixture rename, with no runtime behavior or public API change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
