[chores] Optimized CI build pipeline to speed up feedback #1317#1384
[chores] Optimized CI build pipeline to speed up feedback #1317#1384prathmeshkulkarni-coder wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a set-matrix job that emits a JSON strategy matrix (python-version, django-version, run-selenium) based on event type, introduces a qa job to run code-quality checks on ubuntu-24.04, and updates the build job to consume the emitted matrix via fromJson(). The build job no longer installs global prettier and now gates Selenium execution using matrix.run-selenium; runtests is changed to run Selenium tests only when RUN_SELENIUM=="1". Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
153-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
flag-nameis now broken by the new matrix shape.The rewritten matrix
includeentries only exposepython-versionanddjango-version— there is no longer anenvkey. As a resultmatrix.env.envresolves to an empty string and every parallel coverage upload reports the sameflag-name: python-. Coveralls usesflag-nameto distinguish parallel jobs, so per-combination coverage attribution is lost. Update it to reference the available matrix values.🐛 Proposed fix
- flag-name: python-${{ matrix.env.env }} + flag-name: python-${{ matrix.python-version }}-${{ matrix.django-version }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 153, The CI job's flag-name uses a nonexistent key "python-${{ matrix.env.env }}", causing identical flags for all parallel uploads; update the flag-name to reference the actual matrix fields exposed by the new matrix shape (e.g., use matrix.python-version and/or matrix.django-version). Replace the broken expression with one that combines available variables such as "python-${{ matrix.python-version }}-django-${{ matrix.django-version }}" (or at minimum "python-${{ matrix.python-version }}") so each matrix combination produces a unique flag-name for Coveralls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 16: Add a top-level GitHub Actions permissions block to enforce
least-privilege for the GITHUB_TOKEN (e.g., add permissions: contents: read) and
then grant elevated scopes only in specific jobs that require them; update the
workflow root (where jobs: is defined) to include the permissions: mapping and,
for any job that needs write or other access, add a job-level permissions:
override to explicitly grant those scopes.
- Around line 134-139: The RUN_SELENIUM gating duplicates specific
(matrix.python-version, matrix.django-version) pairs and can drift from the
set-matrix; change the condition that sets RUN_SELENIUM to key only on
matrix.django-version (e.g. check if matrix.django-version matches the supported
Django releases) rather than pairing exact python versions, so Selenium runs for
the intended Django releases regardless of which python version set-matrix
assigns as the first/lowest for that release; update the RUN_SELENIUM expression
that references matrix.python-version and matrix.django-version to only test
matrix.django-version against the allowed Django release strings used in
set-matrix.
- Around line 38-40: Update the QA job's checkout step that uses
actions/checkout@v6 to disable credential persistence by adding
persist-credentials: false under the with block (next to ref: ${{
github.event.pull_request.head.sha }}), so the GITHUB_TOKEN is not written to
local git config during the ./run-qa-checks run.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Line 153: The CI job's flag-name uses a nonexistent key "python-${{
matrix.env.env }}", causing identical flags for all parallel uploads; update the
flag-name to reference the actual matrix fields exposed by the new matrix shape
(e.g., use matrix.python-version and/or matrix.django-version). Replace the
broken expression with one that combines available variables such as "python-${{
matrix.python-version }}-django-${{ matrix.django-version }}" (or at minimum
"python-${{ matrix.python-version }}") so each matrix combination produces a
unique flag-name for Coveralls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91c5f778-42b2-4c6a-bf8f-2f320ed6ee26
📒 Files selected for processing (2)
.github/workflows/ci.ymlruntests
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: QA Checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 38-40: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 17-32: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 34-76: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 25-25: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 43-43: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
17-32: LGTM!runtests (1)
8-13: LGTM!
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
fd543e8 to
fe25bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 40: The checkout step currently uses a PR-only ref value (ref: ${{
github.event.pull_request.head.sha }}), which is empty on non-PR events; change
the ref to an event-safe expression such as ref: ${{
github.event.pull_request.head.sha || github.sha }} (or use github.ref as
appropriate) for both occurrences (the ref keys shown in the workflow) so
actions/checkout receives a valid fallback for push and other triggers.
- Line 38: Replace mutable action tags with immutable commit SHAs for
supply-chain hardening: change actions/checkout@v6, actions/cache@v5,
actions/setup-python@v6, and coverallsapp/github-action@v2 to their
corresponding full commit SHAs (use the commit SHA from each action's official
repository), making sure to update every occurrence (both places each action is
referenced). Locate the usages by the action identifiers (actions/checkout,
actions/cache, actions/setup-python, coverallsapp/github-action) in the
workflow, substitute the @<tag> with @<full-commit-sha>, and run the workflow to
verify the pinned SHAs work correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d66f39c-446f-4051-819e-34682ad4bbe8
📒 Files selected for processing (2)
.github/workflows/ci.ymlruntests
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: QA Checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 38-40: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 17-32: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 34-76: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 25-25: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 43-43: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (1)
runtests (1)
8-13: LGTM!
| name: QA Checks | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify non-SHA-pinned action references in this workflow
rg -nP '^\s*uses:\s+[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+@v[0-9]+' .github/workflows/ci.ymlRepository: openwisp/openwisp-controller
Length of output: 319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
python3 - <<'PY'
import itertools
path=".github/workflows/ci.yml"
start=30; end=70
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i}:{line.rstrip()}")
PY
echo "----"
python3 - <<'PY'
path=".github/workflows/ci.yml"
start=80; end=120
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i}:{line.rstrip()}")
PY
echo "----"
python3 - <<'PY'
path=".github/workflows/ci.yml"
start=135; end=190
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i}:{line.rstrip()}")
PYRepository: openwisp/openwisp-controller
Length of output: 4762
Pin mutable GitHub Actions tags in .github/workflows/ci.yml to full commit SHAs (supply-chain hardening).
Tag-pinned actions detected:
actions/checkout@v6(lines 38, 87)actions/cache@v5(lines 43, 92)actions/setup-python@v6(lines 56, 105)coverallsapp/github-action@v2(lines 149, 162)
What to change
- - uses: actions/checkout@v6
+ - uses: actions/checkout@<full-length-commit-sha>
- uses: actions/cache@v5
+ uses: actions/cache@<full-length-commit-sha>
- uses: actions/setup-python@v6
+ uses: actions/setup-python@<full-length-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 38-40: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 38, Replace mutable action tags with
immutable commit SHAs for supply-chain hardening: change actions/checkout@v6,
actions/cache@v5, actions/setup-python@v6, and coverallsapp/github-action@v2 to
their corresponding full commit SHAs (use the commit SHA from each action's
official repository), making sure to update every occurrence (both places each
action is referenced). Locate the usages by the action identifiers
(actions/checkout, actions/cache, actions/setup-python,
coverallsapp/github-action) in the workflow, substitute the @<tag> with
@<full-commit-sha>, and run the workflow to verify the pinned SHAs work
correctly.
|
coverage: 98.552%. remained the same — prathmeshkulkarni-coder:chore/speedup-ci-1317 into openwisp:master |
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
fe25bed to
8605777
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
146-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix undefined matrix key in Coveralls
flag-name
matrix.env.envis not defined in the job matrix (it only emitspython-version,django-version,run-selenium), soflag-namecan end up empty/unstable and collapse parallel coverage partitions.💡 Proposed fix
- flag-name: python-${{ matrix.env.env }} + flag-name: py${{ matrix.python-version }}-${{ matrix.django-version }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 146 - 153, The Coveralls step "Upload Coverage" uses an undefined matrix key `matrix.env.env` for `flag-name`, causing empty/unstable flags; update the `flag-name` to a stable matrix key such as `python-${{ matrix.python-version }}` (or combine stable keys like `python-${{ matrix.python-version }}-django-${{ matrix['django-version'] }}`) so each parallel job emits a unique, consistent flag and parallel coverage partitions do not collapse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 146-153: The Coveralls step "Upload Coverage" uses an undefined
matrix key `matrix.env.env` for `flag-name`, causing empty/unstable flags;
update the `flag-name` to a stable matrix key such as `python-${{
matrix.python-version }}` (or combine stable keys like `python-${{
matrix.python-version }}-django-${{ matrix['django-version'] }}`) so each
parallel job emits a unique, consistent flag and parallel coverage partitions do
not collapse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 785954bc-50e3-40be-9fa2-ad2d97982c64
📒 Files selected for processing (2)
.github/workflows/ci.ymlruntests
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: QA Checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 41-43: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 90-92: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 28-28: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 46-46: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 59-59: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 90-90: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
20-139: LGTM!runtests (1)
8-13: LGTM!
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
8605777 to
40f1860
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
131-136:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
RUN_SELENIUMenvironment variable missing from Tests step.The PR description states the goal is to "Wrapped Selenium browser test execution in a RUN_SELENIUM=1 conditional so CI can skip Selenium on redundant jobs", and the AI summary claims "Updates the
buildjob "Tests" step env to use matrix-drivenRUN_SELENIUM", but theenv:block at lines 134-136 contains onlySELENIUM_HEADLESSandGECKO_LOG. WithoutRUN_SELENIUMset frommatrix.run-selenium, theruntestsscript cannot conditionally skip Selenium, so all jobs still execute Selenium tests and the 73% reduction claimed in the PR description cannot be achieved.🌐 Add RUN_SELENIUM to Tests step environment
- name: Tests if: ${{ !cancelled() && steps.deps.conclusion == 'success' }} run: ./runtests env: + RUN_SELENIUM: ${{ matrix.run-selenium || '0' }} SELENIUM_HEADLESS: 1 GECKO_LOG: 1This passes the matrix-provided
run-seleniumflag to the test script (defaulting to '0' when absent), enabling selective Selenium execution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 131 - 136, The Tests step is missing the RUN_SELENIUM env value, so the runtests script can't skip Selenium; update the Tests step's env block (where SELENIUM_HEADLESS and GECKO_LOG are set) to include RUN_SELENIUM populated from the matrix (e.g., use matrix.run-selenium with a safe default like '0') so the "Tests" step will receive the matrix-driven flag and conditionally skip Selenium as intended.
144-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid matrix reference
matrix.env.envin coveralls flag-name.Line 150 references
matrix.env.env, but the new matrix structure (lines 30, 33) contains onlypython-version,django-version, and (should contain)run-selenium. The old matrix had a nestedenv.envfield, but that's gone. This will cause the coveralls step to fail with an undefined variable error.🩹 Fix the flag-name to use available matrix variables
- name: Upload Coverage if: ${{ success() }} uses: coverallsapp/github-action@v2 with: parallel: true format: cobertura - flag-name: python-${{ matrix.env.env }} + flag-name: python-${{ matrix.python-version }}-django-${{ matrix.django-version }} github-token: ${{ secrets.GITHUB_TOKEN }} fail-on-error: falseThis uses the actual matrix fields to create a unique flag name for each combination.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 144 - 152, The coveralls step uses a non-existent matrix variable `matrix.env.env`; update the flag-name to use existing matrix fields instead (e.g., `matrix.python-version` and `matrix.django-version`, and include `matrix.run-selenium` if present) so each job gets a unique flag; construct the flag by joining those matrix values with hyphens and prefixing with a clear label like "python-" (ensure you replace the `flag-name: python-${{ matrix.env.env }}` occurrence accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 90-92: The checkout step using actions/checkout@v6 currently omits
persist-credentials, which can leave the GITHUB_TOKEN persisted in .git/config;
update the checkout step (the actions/checkout@v6 step that sets ref: ${{
github.event.pull_request.head.sha || github.sha }}) to include
persist-credentials: false to prevent credential persistence, and mirror this
change for any other checkout steps (e.g., the qa job) to harden against
credential leakage.
- Around line 41-43: The checkout step using actions/checkout@v6 should set
persist-credentials: false to avoid persisting GITHUB_TOKEN into .git/config;
update the checkout step (the block with uses: actions/checkout@v6 and ref: ...)
to include with: persist-credentials: false (alongside the existing ref) so
credentials are not left available to later steps.
- Around line 28-35: The emitted matrix JSON (variable JSON) only contains
python-version and django-version so the build job can't gate Selenium; update
the JSON include entries to add a run-selenium (or RUN_SELENIUM) field set to
"1" for the designated combinations (for PR: mark one combo per Django
release—django~=4.2.0 and django~=5.2.0—on their chosen python-version rows; for
master: mark the first python-version pairing for each Django release 4.2, 5.1,
5.2 with run-selenium="1" and leave other combos as run-selenium="0" or absent),
ensuring the same JSON variable is echoed to GITHUB_OUTPUT so the build job can
read run-selenium to conditionally run Selenium.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 131-136: The Tests step is missing the RUN_SELENIUM env value, so
the runtests script can't skip Selenium; update the Tests step's env block
(where SELENIUM_HEADLESS and GECKO_LOG are set) to include RUN_SELENIUM
populated from the matrix (e.g., use matrix.run-selenium with a safe default
like '0') so the "Tests" step will receive the matrix-driven flag and
conditionally skip Selenium as intended.
- Around line 144-152: The coveralls step uses a non-existent matrix variable
`matrix.env.env`; update the flag-name to use existing matrix fields instead
(e.g., `matrix.python-version` and `matrix.django-version`, and include
`matrix.run-selenium` if present) so each job gets a unique flag; construct the
flag by joining those matrix values with hyphens and prefixing with a clear
label like "python-" (ensure you replace the `flag-name: python-${{
matrix.env.env }}` occurrence accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eedc412a-e7bc-447c-a9b3-b91d324848c1
📒 Files selected for processing (1)
.github/workflows/ci.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: QA Checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 41-43: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 90-92: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 28-28: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 46-46: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 59-59: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 90-90: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
41-41: Pin mutable GitHub Actions tags to full commit SHAs.Tag-pinned actions remain at lines 41, 46, 59, 90, 146, 159. This was flagged in previous reviews.
Also applies to: 46-46, 59-59, 90-90, 146-146, 159-159
16-17: LGTM!
66-79: LGTM!
81-87: LGTM!
115-125: LGTM!
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
40f1860 to
af8cbe4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
153-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix undefined matrix reference in coverage flag-name.
Line 153 references
matrix.env.env, but the emitted matrix only definespython-version,django-version, andrun-seleniumfields. This will evaluate to an empty string, causing all coverage uploads to use the same (empty) flag name and potentially conflate reports.🔧 Proposed fix
- flag-name: python-${{ matrix.env.env }} + flag-name: python-${{ matrix.python-version }}-django-${{ matrix.django-version }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 153, The coverage flag-name uses a non-existent matrix key "matrix.env.env", causing an empty flag; update the flag-name setting to reference existing matrix keys (for example use matrix.python-version or combine matrix.python-version and matrix.django-version) so each job produces a unique flag (e.g., "python-${{ matrix.python-version }}" or "python-${{ matrix.python-version }}-django-${{ matrix.django-version }}") and remove the invalid matrix.env.env reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Line 153: The coverage flag-name uses a non-existent matrix key
"matrix.env.env", causing an empty flag; update the flag-name setting to
reference existing matrix keys (for example use matrix.python-version or combine
matrix.python-version and matrix.django-version) so each job produces a unique
flag (e.g., "python-${{ matrix.python-version }}" or "python-${{
matrix.python-version }}-django-${{ matrix.django-version }}") and remove the
invalid matrix.env.env reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c951f05-62bd-4220-8046-45302a3b112e
📒 Files selected for processing (2)
.github/workflows/ci.ymlruntests
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: QA Checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 28-28: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 47-47: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 60-60: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 91-91: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
41-41: Pin mutable GitHub Actions tags to SHAs for supply-chain hardening.This is the same issue flagged in a previous review:
actions/checkout@v6,actions/cache@v5,actions/setup-python@v6, andcoverallsapp/github-action@v2all use mutable tag references. Consider pinning to full commit SHAs to prevent supply-chain attacks.Also applies to: 47-47, 60-60, 91-91, 97-97, 110-110, 149-149, 162-162
runtests (1)
8-13: LGTM!
CodingWithSaksham
left a comment
CodingWithSaksham
left a comment
There was a problem hiding this comment.
Thanks for working on this! The direction is solid. I do have a few things I'd like to address before the maintainers see this PR.
One important point first: @pandafy commented on issue that selenium should run across Django versions because "CSS styles in Django evolve over time… tests which pass in Django 5.2 may fail in Django 6.2." Your implementation actually runs selenium once per Django version (4.2, 5.1, 5.2 on master), which I think satisfies that but the PR doesn't mention it. Could you reply the thread reconciling this with @pandafy's and include the concrete before/after selenium counts (currently 11 selenium passes per 3 on master / 2 on PRs)? That framing makes the much stronger and avoids it looking like the feedback was missed.
|
One thing that worries me is that a bug that occurs specifically in Python 3.11 and Django 5.2 will only be discovered after the PR is merged into master. Moreover, Django minor releases are known to introduce behavioral changes in QuerySets, form validation, and other areas. Those issues won't be caught until the PR is merged into master. |
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
af8cbe4 to
304e2bd
Compare
Almost all compatibility regressions (syntax, deprecations, form validation changes, or QuerySet behaviors) occur at major version boundaries. By running the oldest supported boundaries (Python 3.10 + Django 4.2) and the newest boundaries (Python 3.13 + Django 5.2) on PRs, we capture 99% of all compatibility regressions before merge. |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Isolated QA checks, implemented a dynamic matrix for PRs, and optimized Selenium tests to run once per supported Django version. Closes openwisp#1317
prathmeshkulkarni-coder
force-pushed
the
chore/speedup-ci-1317
branch
from
304e2bd to
d3dd160
Compare
|
@pandafy here i am not sure about selenium should we run selenium in all version or just 2? |
3 participants
Checklist
Reference to Existing Issue
Closes #1317 .
Description of Changes
github/workflows/ci.yml (CI Workflow Configuration)Isolated QA Checks: Extracted code formatting (prettier), linting (black, flake8, isort), and migration checks into a single independent job. This eliminates the duplicate package setup and lint checking on all 11 matrix environments.
Dynamic Build Matrix: Introduced a dynamically calculated strategy matrix.
On PRs: Only runs 2 boundary tests (Python 3.10 + Django 4.2 and Python 3.13 + Django 5.2) for fast feedback.
On master pushes: Retains the full 11-combination matrix for comprehensive integration checking.
Smart Selenium Triggering: Configured the RUN_SELENIUM: '1' environment variable to trigger only once per Django release version (4.2, 5.1, and 5.2) instead of all 11 combinations.
runtestsConditional Selenium Execution: Refactored the script to wrap the slow Selenium browser test suite in an if block checking for the RUN_SELENIUM=1 environment variable. This allows the CI system to selectively skip Selenium runs on python-redundant jobs, cutting Selenium executions by 73% while keeping full Django compatibility validation.
Screenshot
N/A