Add MSSQL integration test runner workflow#27205
Open
KD23243 wants to merge 5 commits intowso2:masterfrom
Open
Add MSSQL integration test runner workflow#27205KD23243 wants to merge 5 commits intowso2:masterfrom
KD23243 wants to merge 5 commits intowso2:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new GitHub Actions workflow to run MSSQL-based integration tests: matrixed runners, MSSQL 2022 service container, DB/schema setup, optional pack application, JDBC driver install, deployment.toml rewrites, dynamic TestNG filtering, test execution, artifact upload, and optional PR comment+approval. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub Actions
participant Repo as Repository Checkout
participant Runner as Job Runner (ubuntu)
participant MSSQL as MSSQL 2022 Container
participant Maven as Maven Build/Test
participant DB as DB Schema Setup
participant Dist as WSO2 IS Distribution
participant PR as GitHub PR API
GH->>Repo: workflow_dispatch / trigger
Repo->>Runner: checkout code, apply PR diff (optional)
Runner->>MSSQL: start container + healthcheck
Runner->>Maven: setup JDK21, cache, build product (skip IT)
Runner->>Dist: download/unzip pack (optional)
Runner->>DB: install mssql-tools, wait, create databases
DB->>Dist: apply MSSQL schema SQLs
Runner->>Dist: add MSSQL JDBC jar to lib
Runner->>Dist: rewrite repository/conf/deployment.toml
Runner->>Maven: filter testng.xml per matrix, run integration tests
Maven->>GH: upload logs, reports, artifacts
GH->>PR: post comment and approve (if input present and success)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
.github/workflows/.github/workflows/integration-test-runner-mssql.yml (5)
202-239: Potential string injection risk with shell variable interpolation in Python heredoc.The shell variables (e.g.,
${MSSQL_SA_PASSWORD}) are interpolated before being passed to Python. If the password contained quotes or backslashes, it could break the Python string literals or cause unexpected behavior.While the current hardcoded password is safe, this pattern is fragile for future changes. Consider passing values via environment variables and accessing them with
os.environin Python.♻️ Safer approach using os.environ
- name: Configure deployment.toml for MSSQL + env: + MSSQL_SA_PASSWORD: ${{ env.MSSQL_SA_PASSWORD }} + WSO2_IDENTITY_DB: ${{ env.WSO2_IDENTITY_DB }} + WSO2_SHARED_DB: ${{ env.WSO2_SHARED_DB }} + WSO2_AGENT_IDENTITY_DB: ${{ env.WSO2_AGENT_IDENTITY_DB }} + DEPLOYMENT_TOML_PATH: ${{ env.IS_DIST }}/repository/conf/deployment.toml run: | - DEPLOYMENT_TOML="$IS_DIST/repository/conf/deployment.toml" - echo "Configuring $DEPLOYMENT_TOML for MSSQL..." + echo "Configuring $DEPLOYMENT_TOML_PATH for MSSQL..." python3 << PYEOF - import re, os + import re + import os - mssql_sa_password = "${MSSQL_SA_PASSWORD}" - ws_identity_db = "${WSO2_IDENTITY_DB}" - ws_shared_db = "${WSO2_SHARED_DB}" - ws_agent_db = "${WSO2_AGENT_IDENTITY_DB}" - deployment_toml = "${DEPLOYMENT_TOML}" + mssql_sa_password = os.environ["MSSQL_SA_PASSWORD"] + ws_identity_db = os.environ["WSO2_IDENTITY_DB"] + ws_shared_db = os.environ["WSO2_SHARED_DB"] + ws_agent_db = os.environ["WSO2_AGENT_IDENTITY_DB"] + deployment_toml = os.environ["DEPLOYMENT_TOML_PATH"] # ... rest of script PYEOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml around lines 202 - 239, The heredoc currently lets shell interpolate secrets into Python string literals (see mssql_sa_password, ws_identity_db, ws_shared_db, ws_agent_db, deployment_toml and the python heredoc block), which is fragile and can cause injection/escaping issues; instead stop embedding ${...} into the heredoc and read those values from environment variables inside Python via os.environ (e.g., os.environ.get("MSSQL_SA_PASSWORD")), ensure the workflow sets those env vars before invoking the python step (or use the workflow env: context), and update the Python code to use those env values when building the replacement strings for deployment_toml to avoid shell-side interpolation and quoting problems.
301-313: Document or derive the expected BUILD SUCCESS count.The magic number
17for nashorn tests is unexplained and could become outdated if the test module structure changes. Consider adding a comment explaining why 17 is expected, or deriving this count dynamically based on the number of test modules.💡 Add explanatory comment
ENABLED_TESTS="${{ matrix.tests }}" if [[ "$ENABLED_TESTS" == *"is-test-adaptive-authentication-nashorn"* ]]; then + # Nashorn tests trigger multiple sub-module builds (16 modules + 1 parent = 17 successes) EXPECTED_BUILD_SUCCESS_COUNT="17" else EXPECTED_BUILD_SUCCESS_COUNT="1" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml around lines 301 - 313, The script uses a hard-coded EXPECTED_BUILD_SUCCESS_COUNT (17) when ENABLED_TESTS contains "is-test-adaptive-authentication-nashorn"; replace this magic number with a derived value or document it: either compute EXPECTED_BUILD_SUCCESS_COUNT dynamically (e.g., count the number of test modules/artifacts for the selected matrix by parsing the test list or Maven reactor output) or add a clear comment above the conditional explaining exactly why 17 is expected and under which module set it applies; update references to EXPECTED_BUILD_SUCCESS_COUNT and the conditional branch around ENABLED_TESTS to use the new derived variable or the explanatory comment so future changes to module structure won’t break the check.
59-65: Add input validation for PR URL format.The URL parsing using
cutassumes a specific format but doesn't validate it. A malformed URL (e.g., missing/pull/segment, extra slashes, or query parameters) will silently produce incorrect values, causing API calls to fail with unclear errors.Also, this parsing logic is duplicated in multiple steps (lines 59-62, 100-105, 350-353, 360-362). Consider validating the input early and extracting to reusable environment variables.
♻️ Add URL validation and consolidate parsing
Add a validation step early in the workflow:
- name: Validate and parse PR URL if: ${{ github.event.inputs.pr != '' }} id: pr_info run: | PR_LINK="${{ github.event.inputs.pr }}" PR_LINK="${PR_LINK%/}" if [[ ! "$PR_LINK" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then echo "ERROR: Invalid PR URL format. Expected: https://github.com/{owner}/{repo}/pull/{number}" exit 1 fi echo "owner=$(echo "$PR_LINK" | cut -d "/" -f 4)" >> $GITHUB_OUTPUT echo "repo=$(echo "$PR_LINK" | cut -d "/" -f 5)" >> $GITHUB_OUTPUT echo "pr_number=$(echo "$PR_LINK" | cut -d "/" -f 7)" >> $GITHUB_OUTPUTThen reference as
${{ steps.pr_info.outputs.owner }}etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml around lines 59 - 65, The PR URL parsing using cut to derive owner, repo and pr_number (the owner/repo/pr_number variables in the run block) is fragile and duplicated; add a single reusable validation/parsing step (e.g., a job step with id pr_info) that validates the input against a strict regex for https://github.com/{owner}/{repo}/pull/{number}, normalizes/trims trailing slashes, extracts owner/repo/pr_number once and emits them as outputs, and replace the repeated cut invocations with references to those outputs (e.g., steps.pr_info.outputs.owner, steps.pr_info.outputs.repo, steps.pr_info.outputs.pr_number); fail fast with a clear error if validation fails so subsequent curl calls won’t run with malformed values.
92-96: Pin pnpm to a specific version for reproducibility.Using
version: latestcan cause unexpected CI failures if pnpm releases breaking changes. Pin to a specific version to ensure consistent behavior across runs.♻️ Suggested fix
- name: Setup pnpm uses: pnpm/action-setup@v4 with: - version: latest + version: "9" run_install: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml around lines 92 - 96, The CI step "Setup pnpm" currently uses with: version: latest which can introduce breaking changes; change the pnpm/action-setup step (the "Setup pnpm" step that uses pnpm/action-setup@v4) to use a pinned, tested pnpm version instead of "latest" (for example replace version: latest with a specific version string like "7.29.0" or the project-approved pnpm version) to ensure reproducible CI runs.
14-20: Consider using a single source of truth for the SA password.The SA password is defined both as a workflow-level environment variable (line 16) and directly in the service container environment (line 42). If these values diverge, the workflow will fail silently when connecting to MSSQL.
Consider using the environment variable reference in the service definition to maintain a single source of truth, or add a comment noting they must match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml around lines 14 - 20, The SA password is duplicated: MSSQL_SA_PASSWORD is set at workflow env and again hardcoded in the service container; update the service container environment to reference the workflow env instead of repeating the literal (use the workflow env reference for MSSQL_SA_PASSWORD in the service env) so MSSQL_SA_PASSWORD is the single source of truth (or add a clear comment next to both entries enforcing they must match). Locate the MSSQL_SA_PASSWORD entries and replace the hardcoded value in the service environment with the workflow-level variable reference (MSSQL_SA_PASSWORD).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml:
- Line 1: The workflow file named "Integration Test Runner MSSQL" is placed
inside a nested workflows directory and won't be recognized by GitHub Actions;
move this workflow file out of the nested folder into the repository's top-level
workflows directory (so the "Integration Test Runner MSSQL" YAML lives directly
in the recognized workflows folder), ensuring the file name remains
integration-test-runner-mssql.yml and updating any references if needed.
- Around line 363-365: The Authorization header in the curl command that posts
the PR review uses 'Authorization:token' (seen in the curl POST block that calls
https://api.github.com/repos/$owner/$repo/pulls/$pr_number/reviews) and needs a
space after the colon; update that header to 'Authorization: token
${{secrets.PR_BUILDER_COMMENT}}' so it matches the correct HTTP header format
(like the earlier 'Authorization: token' usage) before sending the POST request.
- Around line 160-175: The wait loop that calls sqlcmd (for i in $(seq 1 30) ...
sqlcmd -S localhost,1433 -U SA -P "$MSSQL_SA_PASSWORD" -Q "SELECT 1" -C) does
not fail the job when MSSQL never becomes ready; update the step to detect when
the loop completes without success and exit non‑zero (e.g., set a flag or check
whether the loop broke and call exit 1 with a clear error message like "MSSQL
did not become ready after 30 attempts") before running the subsequent sqlcmd
CREATE DATABASE commands to ensure the workflow stops on readiness failure.
- Around line 271-276: The current loop that disables tests uses sed to append
enabled="false" after name="$test" which can create duplicate enabled
attributes; update the sed invocation used in the loop over ALL_TESTS (refer to
variables ALL_TESTS, ENABLED_TESTS and TESTNG_PATH) to first replace any
existing enabled="..." for that test with enabled="false" and only insert
enabled="false" if no enabled attribute exists (e.g., use a single sed -E
expression that matches name="$test" optionally followed by an enabled attribute
and replaces that whole segment with name="$test" enabled="false"). Ensure the
updated sed runs in-place like the original and still echoes "Disabling: $test".
---
Nitpick comments:
In @.github/workflows/.github/workflows/integration-test-runner-mssql.yml:
- Around line 202-239: The heredoc currently lets shell interpolate secrets into
Python string literals (see mssql_sa_password, ws_identity_db, ws_shared_db,
ws_agent_db, deployment_toml and the python heredoc block), which is fragile and
can cause injection/escaping issues; instead stop embedding ${...} into the
heredoc and read those values from environment variables inside Python via
os.environ (e.g., os.environ.get("MSSQL_SA_PASSWORD")), ensure the workflow sets
those env vars before invoking the python step (or use the workflow env:
context), and update the Python code to use those env values when building the
replacement strings for deployment_toml to avoid shell-side interpolation and
quoting problems.
- Around line 301-313: The script uses a hard-coded EXPECTED_BUILD_SUCCESS_COUNT
(17) when ENABLED_TESTS contains "is-test-adaptive-authentication-nashorn";
replace this magic number with a derived value or document it: either compute
EXPECTED_BUILD_SUCCESS_COUNT dynamically (e.g., count the number of test
modules/artifacts for the selected matrix by parsing the test list or Maven
reactor output) or add a clear comment above the conditional explaining exactly
why 17 is expected and under which module set it applies; update references to
EXPECTED_BUILD_SUCCESS_COUNT and the conditional branch around ENABLED_TESTS to
use the new derived variable or the explanatory comment so future changes to
module structure won’t break the check.
- Around line 59-65: The PR URL parsing using cut to derive owner, repo and
pr_number (the owner/repo/pr_number variables in the run block) is fragile and
duplicated; add a single reusable validation/parsing step (e.g., a job step with
id pr_info) that validates the input against a strict regex for
https://github.com/{owner}/{repo}/pull/{number}, normalizes/trims trailing
slashes, extracts owner/repo/pr_number once and emits them as outputs, and
replace the repeated cut invocations with references to those outputs (e.g.,
steps.pr_info.outputs.owner, steps.pr_info.outputs.repo,
steps.pr_info.outputs.pr_number); fail fast with a clear error if validation
fails so subsequent curl calls won’t run with malformed values.
- Around line 92-96: The CI step "Setup pnpm" currently uses with: version:
latest which can introduce breaking changes; change the pnpm/action-setup step
(the "Setup pnpm" step that uses pnpm/action-setup@v4) to use a pinned, tested
pnpm version instead of "latest" (for example replace version: latest with a
specific version string like "7.29.0" or the project-approved pnpm version) to
ensure reproducible CI runs.
- Around line 14-20: The SA password is duplicated: MSSQL_SA_PASSWORD is set at
workflow env and again hardcoded in the service container; update the service
container environment to reference the workflow env instead of repeating the
literal (use the workflow env reference for MSSQL_SA_PASSWORD in the service
env) so MSSQL_SA_PASSWORD is the single source of truth (or add a clear comment
next to both entries enforcing they must match). Locate the MSSQL_SA_PASSWORD
entries and replace the hardcoded value in the service environment with the
workflow-level variable reference (MSSQL_SA_PASSWORD).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba80d929-9cd0-4cc6-95f9-8f25716cb87f
📒 Files selected for processing (1)
.github/workflows/.github/workflows/integration-test-runner-mssql.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/integration-test-runner-mssql.yml (5)
349-365: Duplicated PR link parsing logic.The PR link parsing (extracting owner, repo, pr_number) is repeated four times throughout the workflow (lines 60-62, 103-105, 351-353, 360-362). Consider extracting this into a reusable step or composite action, or parsing once and passing values via environment variables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner-mssql.yml around lines 349 - 365, The PR owner/repo/pr_number extraction is duplicated in the "Comment build status" and "Approve PR" steps (variables owner, repo, pr_number); consolidate by moving the parsing into a single reusable step (or composite action) that exports owner, repo, and pr_number as environment variables or step outputs, then update the "Comment build status" and "Approve PR" steps to consume those outputs instead of re-parsing the ${{ github.event.inputs.pr }} string.
240-243: Missing verification forAgentIdentitydatasource.The verification step only checks
[database.identity_db]and[database.shared_db]sections, but not the[datasource.AgentIdentity]section that was also modified by the Python script.🔍 Proposed fix to add verification
echo "Verifying deployment.toml MSSQL configuration:" grep -A4 '\[database\.identity_db\]' "$DEPLOYMENT_TOML" grep -A4 '\[database\.shared_db\]' "$DEPLOYMENT_TOML" + grep -A5 '\[datasource\.AgentIdentity\]' "$DEPLOYMENT_TOML"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner-mssql.yml around lines 240 - 243, The workflow verification step is missing a check for the modified `[datasource.AgentIdentity]` section; update the script block that currently greps for '\[database.identity_db\]' and '\[database.shared_db\]' to also grep and print the relevant lines for '\[datasource.AgentIdentity\]' (e.g., add a grep -A<N> '\[datasource.AgentIdentity\]' "$DEPLOYMENT_TOML" similar to the existing checks) so the pipeline verifies the AgentIdentity datasource changes as well.
363-365: Minor inconsistency in Authorization header formatting.Line 364 uses
'Authorization:token ...'(no space after colon) while line 355 uses'Authorization: token ...'(with space). Both should work per HTTP spec, but consistency is preferred.🔧 Proposed fix for consistency
curl -X POST https://api.github.com/repos/$owner/$repo/pulls/$pr_number/reviews \ - -H 'Authorization:token ${{secrets.PR_BUILDER_COMMENT}}' \ + -H 'Authorization: token ${{secrets.PR_BUILDER_COMMENT}}' \ -d '{"body":"Approving the pull request based on successful MSSQL pr build https://github.com/wso2/product-is/actions/runs/${{github.run_id}}","event":"APPROVE"}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner-mssql.yml around lines 363 - 365, The Authorization header in the curl POST that posts PR reviews uses the header string 'Authorization:token ${{secrets.PR_BUILDER_COMMENT}}' which is inconsistent with the earlier 'Authorization: token ...' formatting; update the curl invocation to use the same format with a space after the colon (i.e., 'Authorization: token ${{secrets.PR_BUILDER_COMMENT}}') so the header string in the POST request matches the existing header style and maintains consistency across the workflow.
92-96: Consider pinning pnpm version for reproducibility.Using
version: latestmay cause unexpected CI failures if pnpm introduces breaking changes. Consider pinning to a specific version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner-mssql.yml around lines 92 - 96, The workflow step "Setup pnpm" currently uses pnpm/action-setup@v4 with version: latest which can introduce non-reproducible CI failures; change the step to pin a specific pnpm release (replace version: latest with a concrete version string such as a known stable tag) and keep run_install: false unchanged, so the action uses a fixed pnpm binary for reproducible runs.
301-313: Magic number for expected build success count needs documentation.The expected build success count of
17for Nashorn tests (line 303) is unexplained. This makes the workflow fragile if the number of modules changes. Consider adding a comment explaining why 17 is expected, or making this more robust.Additionally, using string comparison for numeric values (line 309) may cause issues with leading/trailing whitespace from command output. Consider using numeric comparison.
🛠️ Proposed improvement
ENABLED_TESTS="${{ matrix.tests }}" if [[ "$ENABLED_TESTS" == *"is-test-adaptive-authentication-nashorn"* ]]; then + # 17 BUILD SUCCESS messages expected: 16 from sub-modules + 1 from integration module EXPECTED_BUILD_SUCCESS_COUNT="17" else EXPECTED_BUILD_SUCCESS_COUNT="1" fi PR_BUILD_SUCCESS_COUNT=$(grep -o -i "\[INFO\] BUILD SUCCESS" ../../mvn-integration-test.log | wc -l) - if [ "$PR_BUILD_SUCCESS_COUNT" != "$EXPECTED_BUILD_SUCCESS_COUNT" ]; then + if [ "$PR_BUILD_SUCCESS_COUNT" -ne "$EXPECTED_BUILD_SUCCESS_COUNT" ]; then echo "MSSQL Integration test build not successful. Aborting."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner-mssql.yml around lines 301 - 313, The workflow uses a hardcoded magic number EXPECTED_BUILD_SUCCESS_COUNT (set to 17 when matrix.tests contains "is-test-adaptive-authentication-nashorn") and then compares it to PR_BUILD_SUCCESS_COUNT using a string comparison; replace this by either documenting why 17 is expected (add a comment next to EXPECTED_BUILD_SUCCESS_COUNT explaining which modules/tests produce 17 BUILD SUCCESS messages) or, preferably, compute EXPECTED_BUILD_SUCCESS_COUNT dynamically (e.g., count the expected modules/tests for the Nashorn matrix entry instead of hardcoding 17), and change the comparison from string equality to a numeric comparison (use numeric comparison operator like -ne and ensure PR_BUILD_SUCCESS_COUNT is produced without extra whitespace, e.g., use grep -c or trim output before comparing) so the check is robust to whitespace and module count changes; update the references to EXPECTED_BUILD_SUCCESS_COUNT and PR_BUILD_SUCCESS_COUNT accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integration-test-runner-mssql.yml:
- Around line 160-175: The wait loop under the "Wait for MSSQL and create
databases" step can continue to the CREATE DATABASE commands even if MSSQL never
becomes ready; modify the script to detect when the loop exhausts all retries
and fail explicitly (exit non-zero) instead of proceeding. For example, set a
readiness flag (e.g., MSSQL_READY) or check the last sqlcmd result after the for
loop that runs seq 1 30, and if the server never responded, call exit 1 with a
clear error message before the sqlcmd CREATE DATABASE calls for
$WSO2_IDENTITY_DB, $WSO2_SHARED_DB and $WSO2_AGENT_IDENTITY_DB so the job stops
cleanly on timeout.
---
Nitpick comments:
In @.github/workflows/integration-test-runner-mssql.yml:
- Around line 349-365: The PR owner/repo/pr_number extraction is duplicated in
the "Comment build status" and "Approve PR" steps (variables owner, repo,
pr_number); consolidate by moving the parsing into a single reusable step (or
composite action) that exports owner, repo, and pr_number as environment
variables or step outputs, then update the "Comment build status" and "Approve
PR" steps to consume those outputs instead of re-parsing the ${{
github.event.inputs.pr }} string.
- Around line 240-243: The workflow verification step is missing a check for the
modified `[datasource.AgentIdentity]` section; update the script block that
currently greps for '\[database.identity_db\]' and '\[database.shared_db\]' to
also grep and print the relevant lines for '\[datasource.AgentIdentity\]' (e.g.,
add a grep -A<N> '\[datasource.AgentIdentity\]' "$DEPLOYMENT_TOML" similar to
the existing checks) so the pipeline verifies the AgentIdentity datasource
changes as well.
- Around line 363-365: The Authorization header in the curl POST that posts PR
reviews uses the header string 'Authorization:token
${{secrets.PR_BUILDER_COMMENT}}' which is inconsistent with the earlier
'Authorization: token ...' formatting; update the curl invocation to use the
same format with a space after the colon (i.e., 'Authorization: token
${{secrets.PR_BUILDER_COMMENT}}') so the header string in the POST request
matches the existing header style and maintains consistency across the workflow.
- Around line 92-96: The workflow step "Setup pnpm" currently uses
pnpm/action-setup@v4 with version: latest which can introduce non-reproducible
CI failures; change the step to pin a specific pnpm release (replace version:
latest with a concrete version string such as a known stable tag) and keep
run_install: false unchanged, so the action uses a fixed pnpm binary for
reproducible runs.
- Around line 301-313: The workflow uses a hardcoded magic number
EXPECTED_BUILD_SUCCESS_COUNT (set to 17 when matrix.tests contains
"is-test-adaptive-authentication-nashorn") and then compares it to
PR_BUILD_SUCCESS_COUNT using a string comparison; replace this by either
documenting why 17 is expected (add a comment next to
EXPECTED_BUILD_SUCCESS_COUNT explaining which modules/tests produce 17 BUILD
SUCCESS messages) or, preferably, compute EXPECTED_BUILD_SUCCESS_COUNT
dynamically (e.g., count the expected modules/tests for the Nashorn matrix entry
instead of hardcoding 17), and change the comparison from string equality to a
numeric comparison (use numeric comparison operator like -ne and ensure
PR_BUILD_SUCCESS_COUNT is produced without extra whitespace, e.g., use grep -c
or trim output before comparing) so the check is robust to whitespace and module
count changes; update the references to EXPECTED_BUILD_SUCCESS_COUNT and
PR_BUILD_SUCCESS_COUNT accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38ca4d2c-8e2a-4cdf-9976-a9eea245afbd
📒 Files selected for processing (1)
.github/workflows/integration-test-runner-mssql.yml
|
Malith-19
reviewed
Mar 24, 2026
| - name: Print Input | ||
| run: echo "Running the Test Runner ${{ matrix.runner_id }} for PR - ${{ github.event.inputs.pr }}" | ||
|
|
||
| - name: Comment build info |
Contributor
There was a problem hiding this comment.
Check other steps also if they are depend on the PR link input too.
Malith-19
reviewed
Mar 24, 2026
Comment on lines
+92
to
+96
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: latest | ||
| run_install: false |
Contributor
There was a problem hiding this comment.
since we are not supporting identity apps we don't need the pnpm related steps.
Malith-19
reviewed
Mar 24, 2026
Comment on lines
+29
to
+36
| - runner_id: 1 | ||
| tests: "is-tests-default-configuration,is-test-rest-api,is-test-webhooks,is-tests-scim2,is-test-adaptive-authentication" | ||
| - runner_id: 2 | ||
| tests: "is-test-adaptive-authentication-nashorn,is-test-adaptive-authentication-nashorn-with-restart,is-tests-default-configuration-ldap,is-tests-uuid-user-store,is-tests-federation,is-tests-federation-restart" | ||
| - runner_id: 3 | ||
| tests: "is-tests-jdbc-userstore,is-tests-read-only-userstore,is-tests-oauth-jwt-token-gen-enabled,is-tests-email-username,is-tests-saml-query-profile,is-tests-default-encryption,is-test-session-mgt,is-tests-password-update-api" | ||
| - runner_id: 4 | ||
| tests: "is-tests-with-individual-configuration-changes" |
Contributor
There was a problem hiding this comment.
If we are not running this action frequently we can run it in a single runner. only concern is if we are encouraging people to run this before PR merging, then it's fine.
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.
Summary
The new workflow,
integration-test-runner-mssql.yml, enables automated testing of PRs specifically for MSSQL compatibility. It automates the entire lifecycle, from spinning up an MSSQL Docker container and applying schemas to running a partitioned suite of integration tests across parallel runners.Summary by CodeRabbit