terrymanu commented on PR #40311: URL: https://github.com/apache/shardingsphere/pull/40311#issuecomment-6314301827
### Result Review Result: Mergeable No blocking issues were found. The 93 replacements preserve behavior for the existing Bash invocation paths and supported inputs, without expanding scope. The original `[ … ]` syntax was also valid; this PR standardizes conditional syntax. ### Evidence - All four scripts already require Bash. String-comparison RHS quoting is preserved, numeric operands come from controlled integer-producing paths, and the surrounding `&&`, `||`, grouping, and exit paths remain unchanged. - The complete matrix regression script was rerun during this review: exit code 0, with 167 assertions passed and 0 failed. - Base-versus-PR comparisons produced identical output for five matrix-generation inputs and three remaining-matrix branches. With a simulated collection failure, both versions exited with code 2 rather than incorrectly reporting no jobs. - `bash -n` exited with code 0 for all four files. `git diff --check` from the baseline to the reviewed commit also exited with code 0. ### Coverage Public PR: [apache/shardingsphere#40311](https://github.com/apache/shardingsphere/pull/40311). Head: `bf70dceb83c50a3714689813a16a169146711dbf` Merge-base: `7e63199751c4c5143f4fd82fbd10ea04bf77bb1a` The complete GitHub file list and public blob identities match the local candidate, with no additional local changes. | Fully reviewed file | Conditional replacements | |---|---:| | `.github/workflows/resources/scripts/e2e-sql/generate-e2e-sql-matrix.sh` | 83 | | `.github/workflows/resources/scripts/e2e-sql/generate-remaining-matrix.sh` | 2 | | `.github/workflows/resources/scripts/e2e-sql/test-generate-e2e-sql-matrix.sh` | 6 | | `.github/workflows/resources/scripts/unit-test-coverage-merge/code-coverage-merge.sh` | 2 | The requirement is to standardize existing Bash conditional syntax while preserving behavior. The review covered four behavior clusters: matrix generation, remaining-matrix subtraction, regression assertions, and coverage-input checks. No new APIs, compatibility guarantees, or project support commitments are introduced. All 55 changed blocks were reviewed through three lenses—root cause and behavior, impact and contracts, and tests and runtime—and a final convergence pass. Two rounds, each containing two isolated assessments, were completed. After checking every candidate issue and its counter-evidence, the conclusions agreed, with no unresolved differences or decisive evidence gaps. Session validation exited with code 0. - **Checkstyle: Not Applicable.** The four `.sh` files are outside the extension scope in [`checkstyle.xml:22`](https://github.com/apache/shardingsphere/blob/bf70dceb83c50a3714689813a16a169146711dbf/src/resources/checkstyle.xml#L22). `checkstyle:check` was not run; exit code N/A. - **Spotless: Not Applicable.** The four files are outside the formatting scope in [`pom.xml:1354`](https://github.com/apache/shardingsphere/blob/bf70dceb83c50a3714689813a16a169146711dbf/pom.xml#L1354). `spotless:check` was not run; exit code N/A. This conclusion covers code correctness only. CI was not reviewed, and the actual coverage aggregation workflow was not executed. No code was modified and no comments were posted to GitHub during this review. -- 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]
