terrymanu commented on PR #38795: URL: https://github.com/apache/shardingsphere/pull/38795#issuecomment-4619238061
### Decision Merge Verdict: Mergeable I did not find any merge-blocking issues. This PR centralizes MCP E2E LLM runtime, model, and MySQL image metadata in `e2e-env.properties`; the workflow, local Docker build script, and Java test configuration now read from the same source. I also did not find remaining hardcoded image digests in the workflow/Dockerfile or placeholder model names in the reviewed MCP scope. ### Reviewed Scope - PR: apache/shardingsphere#38795 - Title: Centralize runtime image metadata - Latest PR head SHA: `e54ecdfa083de5e92bbdbda6773eac815709357c` - Current base SHA from GitHub API: `440a930fc7687baf9ece33f48243bf85cb3691d9` - Local merge-base SHA: `ecb7c87c9da4e20986e993a3eb303176b5fd4bcd` - Local checkout matched PR head: `HEAD == e54ecdfa083de5e92bbdbda6773eac815709357c` - GitHub `/pulls/38795/files` matched the local triple-dot diff file list exactly. Files reviewed: - `.github/workflows/e2e-mcp.yml` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/config/LLME2EConfiguration.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/config/LLME2EConfigurationTest.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/artifact/LLME2EArtifactWriter.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/artifact/LLME2EArtifactWriterTest.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/client/LLMChatModelClientTest.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/fixture/LLMRuntimeSupport.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/fixture/LLMRuntimeSupportTest.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/runtime/MySQLRuntimeTestSupport.java` - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/runtime/MySQLRuntimeTestSupportTest.java` - `test/e2e/mcp/src/test/resources/docker/llm-runtime/Dockerfile` - `test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh` - `test/e2e/mcp/src/test/resources/env/e2e-env.properties` ### Findings No blocking findings. ### Review Notes - The workflow now loads MCP E2E image and model metadata from `test/e2e/mcp/src/test/resources/env/e2e-env.properties` and passes those values into Docker build args. - The local `build-local.sh` uses the same property source and override model as the workflow, so local reproduction and GitHub Actions are aligned. - `LLME2EConfiguration` now owns model metadata through configuration instead of keeping fixed architecture-specific digest constants in Java code. - `LLMRuntimeSupport` now serves the configured prepackaged model path and records runtime/model metadata in score evidence. - `MySQLRuntimeTestSupport` now reads the MySQL Testcontainers image from MCP E2E env properties, matching the existing E2E configuration style. - I checked the previous placeholder-name concern: the default and built image labels still use `ggml-org/Qwen3-1.7B-GGUF:Q4_K_M`, not `foo/model`, `Q4_TEST`, or `test-model`. ### Local Verification Commands run locally: - `curl -s https://api.github.com/repos/apache/shardingsphere/pulls/38795` Exit code: 0. Confirmed latest head SHA `e54ecdfa083de5e92bbdbda6773eac815709357c`. - `curl -s 'https://api.github.com/repos/apache/shardingsphere/pulls/38795/files?per_page=100'` compared with local triple-dot diff Exit code: 0. GitHub file list matched local file list. - `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dtest=LLME2EConfigurationTest,LLME2EArtifactWriterTest,LLMChatModelClientTest,LLMRuntimeSupportTest,MySQLRuntimeTestSupportTest -Dsurefire.failIfNoSpecifiedTests=false test` Exit code: 0. MCP touched tests passed: 23 tests, 0 failures, 0 errors. - `sh test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh --print` Exit code: 0. Confirmed properties expand to the expected Qwen model metadata. - `docker buildx build --check ... test/e2e/mcp/src/test/resources/docker/llm-runtime` Exit code: 0. Dockerfile check completed with no warnings. - `sh test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh` Exit code: 0. Local image build succeeded using the cached model layer and wrote image `sha256:7598f889f44afa0da63c782890b7a8875817e7abad79ce80f1abc2febbdad228`. - `docker image inspect apache/shardingsphere-mcp-llm-runtime:local --format '{{json .Config.Labels}}'` Exit code: 0. Labels include `ggml-org/Qwen3-1.7B-GGUF:Q4_K_M`, `Q4_K_M`, revision `daeb8e2d528a760970442092f6bf1e55c3b659eb`, and SHA-256 `d2387ca2dbfee2ffabce7120d3770dadca0b293052bc2f0e138fdc940d9bc7b5`. ### Residual Risk This review did not rely on CI status. Local verification covered the changed configuration path, selected MCP unit tests, Dockerfile syntax checks, and an actual cached local runtime image build. I do not see remaining in-scope risk that should block merge. -- 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]
