terrymanu commented on PR #38791:
URL: https://github.com/apache/shardingsphere/pull/38791#issuecomment-4618502744
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR #38791 latest head
`7e39959b090fbb29653f35d64e20f001d2a29e02`, base/merge-base
`25427fab94c911f6245f9d5ac2554710c08bb46c`; reviewed 31 changed files under MCP
workflows, MCP E2E docs, `test/e2e/mcp`, and related MCP E2E support tests.
GitHub `/pulls/38791/files` matched the local triple-dot diff file list.
- **Not Reviewed Scope:** GitHub Actions / CI status, unrelated non-MCP
modules, and completed local full Docker E2E as a pass signal.
- **Need Expert Review:** No.
### Basis
- The root-cause fix is direct: `.github/workflows/e2e-mcp.yml:91` now runs
`test/e2e/mcp` with `-Dtest='*E2ETest'`, `-De2e.run.type=DOCKER`, and the MCP
distribution image property, so new MCP `*E2ETest` classes are no longer
omitted by a hand-maintained class list.
- The old MCP-specific enable flags and `llm-e2e` profile are removed from
the latest head. A repository scan found no remaining `MCP_E2E_TESTS`,
`mcp.e2e.*.enabled`, `mcp.e2e.llm.enabled`, or
`MCP_LLM_BASE_SERVER_IMAGE_DIGEST` references in the reviewed MCP
workflow/test/doc scope.
- The configuration path now follows the existing E2E env-loader pattern:
`MCPE2ETestConfiguration` loads `EnvironmentPropertiesLoader.loadProperties()`
at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/env/MCPE2ETestConfiguration.java:30`,
and Docker enablement is derived from `e2e.run.type` at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/env/MCPE2ETestConfiguration.java:38`.
- The default disabled path is preserved:
`test/e2e/mcp/src/test/resources/env/e2e-env.properties:20` keeps
`e2e.run.type` empty by default, while system properties can still override it
through the shared loader.
- The release workflow is consistent with the same enablement model:
`.github/workflows/release-mcp.yml:102` and
`.github/workflows/release-mcp.yml:160` use `-De2e.run.type=DOCKER` instead of
a separate distribution-only flag.
- The PR does not touch SQL parser logic, name resolution, routing
precedence, metadata assembly for production runtime, or Proxy/JDBC
high-frequency DML/DQL execution paths, so there is no cross-dialect parser
risk or `ConcurrentHashMap#computeIfAbsent` hot-path risk introduced by this
change.
- No substantive unrelated changes were found in the GitHub-authoritative PR
scope.
### Verification
- Verified PR boundary with GitHub `/pulls/38791/files` vs local triple-dot
diff: matched, exit code `0`.
- Ran focused local verification with dependent modules built from the same
PR head:
- `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true
-Dtest=MCPE2ETestConfigurationTest,LLME2EConfigurationTest,LLMMCPModelFacingToolResponseFormatterTest,PackagedDistributionTestSupportTest
test -Dsurefire.failIfNoSpecifiedTests=false -B -ntp`
- Result: exit code `0`; 19 focused tests passed, 0 failures, 0 errors, 0
skipped.
- Ran the disabled-path wildcard counterexample:
- `./mvnw -pl test/e2e/mcp -DskipITs -Dspotless.skip=true
-Dtest='*E2ETest' -Dsurefire.failIfNoSpecifiedTests=true test -B -ntp`
- Result: exit code `0`; 136 MCP E2E test cases were discovered and
skipped with default empty `e2e.run.type`, confirming the wildcard does not
accidentally run Docker E2E by default.
- I did not use GitHub Actions / CI status for this verdict.
--
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]
