sadpandajoe commented on code in PR #44699:
URL: https://github.com/apache/superset/pull/44699#discussion_r4133227534
##########
.github/workflows/codeql-analysis.yml:
##########
@@ -37,6 +35,21 @@ jobs:
uses: $/.github/actions/change-detector/
with:
token: ${{ secrets.GITHUB_TOKEN }}
+ - name: Dynamically setup matrix of changed languages
+ id: set-lang-matrix
+ shell: bash
+ run: |
+ lang_matrix_string=''
+ if [[ "${STEPS_CHECK_OUTPUTS_PYTHON}" == "true" ]]; then
+ lang_matrix_string=${lang_matrix_string}'"python"'
+ fi
+ if [[ "${STEPS_CHECK_OUTPUTS_FRONTEND}" == "true" ]]; then
+ lang_matrix_string=${lang_matrix_string}'"javascript"'
+ fi
+ echo "lang_matrix=$(jq --compact-output --null-input '[inputs] |
tostring' <<< $lang_matrix_string)"
Review Comment:
Even after this step is wired up to `$GITHUB_OUTPUT`, the matrix still won't
build correctly:
1. `changes.outputs` above only declares `python` and `frontend` — there's
no `lang_matrix` entry, so `needs.changes.outputs.lang_matrix` in the `analyze`
job stays empty no matter what this step writes.
2. `jq '[inputs] | tostring'` serializes the array into a JSON string (e.g.
`"[\"python\",\"javascript\"]"`), not a bare array. `fromJson(...)` on that
value returns a string, not the list `matrix.language` needs.
Could you add `lang_matrix: ${{ steps.set-lang-matrix.outputs.lang_matrix
}}` to this job's `outputs:` block and drop `| tostring` so the step emits a
plain JSON array?
##########
.github/workflows/codeql-analysis.yml:
##########
@@ -56,7 +69,7 @@ jobs:
strategy:
fail-fast: false
matrix:
- language: ["python", "javascript"]
+ language: ${{ fromJson(needs.changes.outputs.lang_matrix) }}
Review Comment:
This narrows CodeQL coverage versus the previous hardcoded matrix:
`python`/`frontend` are directory-based change-detector groups, not a language
classifier. A change limited to a `.js` file living outside
`superset-frontend/` — e.g. `superset/mcp_service/index.js`, or a `.js` file
under `scripts/` — sets `python=true` and leaves `frontend=false`, so this
matrix will only run the `python` CodeQL analysis and silently skip
`javascript`, even though a JavaScript file changed. The old `["python",
"javascript"]` matrix always ran both. Is that coverage gap intentional, or
should the matrix reflect the actual language of the changed files rather than
reusing the `changes`/`frontend` groups?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
