Feature/improve ci#314
Conversation
WalkthroughUpdated CI workflow and tooling: broadened triggers and manual dispatch, set global PYTHON_VERSION=3.11, added permissions and concurrency; upgraded setup-python to v5, added caching, Ruff formatting, pytest-xdist, a Bandit security job with artifact upload, expanded release/publish flows, added Bandit config in pyproject.toml, and simplified mkdir logic in flixopt/calculation.py. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Events
participant WF as python-app.yaml
participant L as Lint (Ruff)
participant T as Test (pytest-xdist)
participant S as Security (Bandit)
participant R as Release (gh-release@v2)
participant P as Publish (TestPyPI/PyPI)
participant D as Docs (deploy-docs)
Dev->>GH: push / pull_request / workflow_dispatch
GH->>WF: trigger (env.PYTHON_VERSION=3.11, permissions, concurrency)
par Parallel jobs
WF->>L: setup-python@v5 → cache → ruff check (--output-format=github) → ruff format
WF->>T: setup-python@v5 → cache deps → install deps (includes pytest-xdist) → pytest --numprocesses=auto
WF->>S: setup-python@v5 → cache → install bandit[toml] → bandit (JSON) → upload artifact
end
alt release conditions met
WF->>R: Create GitHub Release (softprops/action-gh-release@v2, generate_release_notes)
R->>P: build & upload (environment: testpypi/pypi) → verification steps
end
opt docs deployment
WF->>D: build docs (cached deps) & deploy (permissions: contents: write)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
✨ Finishing touches🧪 Generate unit tests
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
🧹 Nitpick comments (6)
pyproject.toml (1)
154-156: B101 skip is redundant given tests are already excluded.Since
exclude_dirs = ["tests/"], you can dropB101and let Bandit flagassertin non-test code.[tool.bandit] -skips = ["B101"] +skips = [].github/workflows/python-app.yaml (5)
91-103: Align runner image for determinism.Other jobs pin
ubuntu-22.04; consider pinning security to the same to avoid drift.- runs-on: ubuntu-latest + runs-on: ubuntu-22.04
150-157: Avoid mixing body_path with generate_release_notes.action-gh-release treats auto notes as fallback; with body_path present, auto-notes are redundant. Pick one.
- name: Create GitHub Release uses: softprops/action-gh-release@v2 with: body_path: current_release_notes.md draft: false prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') || contains(github.ref, 'rc') }} - generate_release_notes: true
139-149: Remove no-op “Sync changelog to docs” in release job.Copying into the workspace doesn’t persist; the deploy-docs job already handles this.
- - name: Sync changelog to docs - run: | - cp CHANGELOG.md docs/changelog.md - echo "✅ Synced changelog to docs"
257-293: Docs push requires write permission and newline at EOF.
- Ensure this job has
permissions.contents: write(see earlier diff).- YAML lint: add trailing newline after Line 293.
- mike set-default --push latest + mike set-default --push latest +
11-13: Consider removing unused release trigger.Workflow listens to
release: createdbut the release job runs on tag pushes. Drop the release trigger to avoid confusion.release: - types: [created] # Trigger when a release is created + # Removed to avoid duplicate/confusing triggers + # types: [created]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/python-app.yaml(10 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/python-app.yaml
[error] 293-293: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (6)
pyproject.toml (1)
154-156: Bandit 'skips' honored; 'exclude_dirs' not yet verifiedbandit 1.8.6 (with bandit[toml]) read pyproject.toml and applied profile exclude tests B506,B101 when scanning flixopt; run bandit -r . -c pyproject.toml (or confirm CI uses bandit[toml]) to verify tests/ is excluded.
.github/workflows/python-app.yaml (5)
5-10: LGTM: Broader branch filters.Running on main/dev and common feature/hotfix prefixes is sensible for CI coverage.
20-23: LGTM: Concurrency guard.Cancel-in-progress avoids needless parallel runs on the same ref.
35-38: LGTM: actions/setup-python upgraded to v5 with centralized version.
50-55: LGTM: Ruff formatting check with grouped logs.
73-82: LGTM: Pip cache keyed by Python version and pyproject.
| - name: Run Bandit security scan | ||
| run: | | ||
| bandit -r flixopt/ -f json -o bandit-report.json || true | ||
| bandit -r flixopt/ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Bandit gating intentional; avoid duplicate scans and flaky failures.
Current double-run writes JSON (non-fatal) then runs again (fatal on any finding). Suggest: single gating run failing only on HIGH severity+confidence, plus a non-fatal human-readable summary.
- - name: Run Bandit security scan
- run: |
- bandit -r flixopt/ -f json -o bandit-report.json || true
- bandit -r flixopt/
+ - name: Run Bandit security scan
+ run: |
+ # Gate on HIGH severity & HIGH confidence; produce JSON artifact
+ bandit -r flixopt/ -f json -o bandit-report.json -q -lll -ii
+ # Human-readable output without affecting job status
+ bandit -r flixopt/ -q --exit-zero📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run Bandit security scan | |
| run: | | |
| bandit -r flixopt/ -f json -o bandit-report.json || true | |
| bandit -r flixopt/ | |
| - name: Run Bandit security scan | |
| run: | | |
| # Gate on HIGH severity & HIGH confidence; produce JSON artifact | |
| bandit -r flixopt/ -f json -o bandit-report.json -q -lll -ii | |
| # Human-readable output without affecting job status | |
| bandit -r flixopt/ -q --exit-zero |
🤖 Prompt for AI Agents
.github/workflows/python-app.yaml around lines 109-113: the workflow currently
runs Bandit twice (one non-fatal JSON run then a full run that fails on any
finding), which causes duplicate scans and flaky failures; replace the two-step
invocation with a single Bandit run that outputs machine-readable JSON
(bandit-report.json), then add a short post-step that parses that JSON and fails
the job only if any finding has severity "HIGH" and confidence "HIGH", while
also printing a non-fatal human-readable summary of all findings for developers;
implement the parser as a tiny shell/Python/JQ step that exits with non-zero
only when a HIGH/HIGH finding is present.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python-app.yaml (1)
227-237: Derive package name robustly (hyphenated names break current sed).Parse pyproject to avoid truncating names containing dashes.
- # Get the package name from the built distribution - PACKAGE_NAME=$(ls dist/*.tar.gz | head -n 1 | sed 's/dist\///' | sed 's/-[0-9].*$//') + # Get project name from pyproject.toml (PEP 621) + PACKAGE_NAME=$(python - <<'PY' +import sys, tomllib, pathlib +data = tomllib.loads(pathlib.Path("pyproject.toml").read_text(encoding="utf-8")) +print(data["project"]["name"]) +PY +)Note:
tomllibis in Python 3.11+.
♻️ Duplicate comments (3)
.github/workflows/python-app.yaml (3)
15-18: Top-level read-only permissions are good; job-level elevation applied elsewhere.Matches least-privilege. Nothing to change.
147-149: Release job correctly elevates to contents: write.Fixes the prior “read-only token” issue for releases.
301-303: Docs job correctly elevates to contents: write.Enables mike pushes to gh-pages.
🧹 Nitpick comments (5)
.github/workflows/python-app.yaml (5)
136-143: Preserve reports longer.Consider explicit artifact retention for forensics.
- name: Upload security reports uses: actions/upload-artifact@v4 if: always() with: name: security-report path: | bandit-report.json + retention-days: 14
285-296: PyPI index propagation can lag; add retry loop.Reduce flakiness by retrying installs.
- # Wait for PyPI to index the package - sleep 60 - # Install from PyPI - pip install $PACKAGE_NAME + # Wait and retry while PyPI indexes the package + for d in 30 60 120; do + sleep "$d" + pip install "$PACKAGE_NAME" && break || true + done
39-47: Simplify pip caching via setup-python’s built-in cache.Fewer moving parts; automatic invalidation with dependency files.
- - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: ${{ env.PYTHON_VERSION }} - - # Cache pip dependencies for linting tools - - name: Cache pip dependencies - uses: actions/cache@v4 - with: - path: ~/.cache/pip - key: ${{ runner.os }}-lint-pip-${{ hashFiles('**/pyproject.toml') }} - restore-keys: | - ${{ runner.os }}-lint-pip- + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: ${{ env.PYTHON_VERSION }} + cache: 'pip' + cache-dependency-path: | + pyproject.tomlRepeat similarly for test/build jobs, replacing manual cache steps.
Also applies to: 82-93, 203-209, 259-267
346-346: Add a trailing newline to satisfy yamllint.Prevents CI lint noise.
- mike set-default --push latest + mike set-default --push latest +
168-181: Use generated release notes or a body file — don’t use both.Verified: scripts/extract_release_notes.py exists (defines extract_release_notes and main). Choose one:
- Option A — Use generated notes: remove the Extract release notes step and remove body_path from the Create GitHub Release step.
- Option B — Use custom notes: keep the Extract release notes step (body_path: current_release_notes.md) and set generate_release_notes: false.
Suggested diff for Option A:
- - name: Extract release notes - run: | - VERSION=${GITHUB_REF#refs/tags/v} - echo "Extracting release notes for version: $VERSION" - python scripts/extract_release_notes.py $VERSION > current_release_notes.md - - name: Create GitHub Release uses: softprops/action-gh-release@v2 with: - body_path: current_release_notes.md draft: false prerelease: ${{ contains(github.ref, 'alpha') || contains(github.ref, 'beta') || contains(github.ref, 'rc') }} generate_release_notes: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-app.yaml(10 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/python-app.yaml
[error] 346-346: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (4)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flixopt/calculation.py (1)
65-70: Guard against “path exists but is not a directory”.If self.folder exists as a file (or a non-dir path element), mkdir will raise FileExistsError/NotADirectoryError with a less helpful message. Add an explicit check and surface a clearer error.
Apply:
try: - self.folder.mkdir(parents=False, exist_ok=True) + if self.folder.exists() and not self.folder.is_dir(): + raise NotADirectoryError(f'Path {self.folder} exists and is not a directory.') + self.folder.mkdir(parents=False, exist_ok=True) except FileNotFoundError as e: raise FileNotFoundError( f'Folder {self.folder} and its parent do not exist. Please create them first.' ) from eOptional: consider reusing fx_io.CalculationResultsPaths(self.folder, self.name).create_folders(parents=False) to keep folder-creation behavior/messages consistent across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flixopt/calculation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/calculation.py (2)
flixopt/io.py (3)
update(300-308)CalculationResultsPaths(257-308)create_folders(287-298)flixopt/results.py (2)
__init__(128-163)from_file(62-100)
⏰ 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). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (1)
flixopt/calculation.py (1)
65-70: Good switch to EAFP and race-free mkdir.Using Path.mkdir(exist_ok=True) inside try/except removes the TOCTOU window and is concurrency-safe. Nice.
Docstrings generation was requested by @FBumann. * #314 (comment) The following files were modified: * `flixopt/calculation.py`
|
Note Generated docstrings for this pull request at #315 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
.github/workflows/python-app.yaml (8)
37-45: Simplify caching with setup-python’s built-in pip cache.Apply:
- - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: ${{ env.PYTHON_VERSION }} - - # Cache pip dependencies for linting tools - - name: Cache pip dependencies - uses: actions/cache@v4 - with: - path: ~/.cache/pip - key: ${{ runner.os }}-lint-pip-${{ hashFiles('**/pyproject.toml') }} - restore-keys: | - ${{ runner.os }}-lint-pip- + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: ${{ env.PYTHON_VERSION }} + cache: 'pip' + cache-dependency-path: '**/pyproject.toml'
80-91: Cache tweak.Caching .pytest_cache rarely helps and can be noisy; rely on pip cache instead via setup-python.
127-133: Bandit: split gating and human-readable runs so output always prints.Currently both run in one step; if the gating command fails, the readable output won’t execute. Split into two steps and mark the second with if: always().
- - name: Run Bandit security scan - run: | - # Gate on HIGH severity & MEDIUM confidence; produce JSON artifact - bandit -r flixopt/ -c pyproject.toml -f json -o bandit-report.json -q -lll -ii - # Human-readable output without affecting job status - bandit -r flixopt/ -c pyproject.toml -q --exit-zero + - name: Run Bandit (JSON, gating) + run: | + # Gate on HIGH severity & MEDIUM confidence; produce JSON artifact + bandit -r flixopt/ -c pyproject.toml -f json -o bandit-report.json -q -lll -ii + - name: Run Bandit (human-readable, non-fatal) + if: always() + run: | + bandit -r flixopt/ -c pyproject.toml -q --exit-zeroAlso confirm MEDIUM confidence gating is intentional (earlier discussion mentioned HIGH/HIGH).
174-180: Release notes: avoid mixing body_path with generate_release_notes.Pick one to prevent duplicate/unexpected content.
- generate_release_notes: true + # Use body_path (custom extractor) and disable auto-notes to avoid mixing sources + generate_release_notes: false
197-209: Reuse built-in pip cache here too.- - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: ${{ env.PYTHON_VERSION }} - - # Cache build tools - - name: Cache build tools - uses: actions/cache@v4 - with: - path: ~/.cache/pip - key: ${{ runner.os }}-build-pip-${{ hashFiles('**/pyproject.toml') }} - restore-keys: | - ${{ runner.os }}-build-pip- + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: ${{ env.PYTHON_VERSION }} + cache: 'pip' + cache-dependency-path: '**/pyproject.toml'
259-271: Same cache simplification applies here.
326-334: Docs deps cache can also use setup-python cache.
219-225: Consider PyPI “Trusted Publishers” (OIDC) instead of API tokens.Eliminates long-lived secrets; uses short-lived OIDC tokens from GitHub. Requires PyPI project config once.
Also applies to: 281-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/python-app.yaml(10 hunks)flixopt/calculation.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flixopt/calculation.py
⏰ 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: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
🔇 Additional comments (17)
.github/workflows/python-app.yaml (17)
5-11: Triggers look good; confirm PR branch filter intent.PR branch filters apply to the base (target) branch. If you meant “PRs originating from feature/* etc. into main/dev,” this won’t match; keep only [main, dev] or add more base branches intentionally.
13-16: Least-privilege default is correct.Top-level contents: read with per-job elevation is the right balance.
18-20: Good use of concurrency.Prevents duplicate runs on the same ref.
23-23: PYTHON_VERSION env set to 3.11 is fine.Matrix still covers 3.10–3.13; env is only used in non-matrix jobs.
33-36: Upgrade to setup-python v5 is correct.
52-56: Ruff lint grouping and GH format: nice.
57-62: Ruff formatting check: nice addition.
76-76: Matrix setup-python v5: good.
95-95: Dev deps + xdist install is fine.
98-99: Parallel pytest usage looks good.
144-148: Per-job write permissions on release: good.
158-160: Release job Python setup via env is fine.
189-191: Environments for TestPyPI: good practice.
231-237: Tomllib-based package name extraction: neat and reliable.
250-253: PyPI environment metadata: good.
294-299: Tomllib reuse here is fine.
310-325: Docs job permissions and setup: looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python-app.yaml (1)
191-197: Twine to TestPyPI likely fails without .pypirc — use explicit repository URL.GitHub runners don’t have a [testpypi] entry by default. Point Twine at the legacy URL.
- twine upload --repository testpypi dist/* --verbose + twine upload --repository-url https://test.pypi.org/legacy/ dist/* --verboseOptionally, consider pypa/gh-action-pypi-publish for both TestPyPI and PyPI to simplify auth and retries.
🧹 Nitpick comments (4)
.github/workflows/python-app.yaml (4)
45-56: Pin Ruff to avoid unexpected CI breakages; print version for traceability.Apply:
- name: Install Ruff run: | python -m pip install --upgrade pip - pip install ruff + pip install "ruff==0.6.*" + ruff --version
57-65: Add a timeout to the test job to prevent hung workflows.test: - runs-on: ubuntu-22.04 + runs-on: ubuntu-22.04 + timeout-minutes: 30
85-89: Standardize runner image for determinism.Security uses ubuntu-latest while other jobs use ubuntu-22.04. Align to a fixed image to avoid surprise upgrades.
- runs-on: ubuntu-latest + runs-on: ubuntu-22.04
140-144: Remove redundant changelog sync in release job.This copy isn’t used (no push happens here) and is duplicated in deploy-docs. Drop it to reduce churn.
- - name: Sync changelog to docs - run: | - cp CHANGELOG.md docs/changelog.md - echo "✅ Synced changelog to docs"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/python-app.yaml(9 hunks)
⏰ 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). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
🔇 Additional comments (6)
.github/workflows/python-app.yaml (6)
5-13: Triggers rationalization looks good — no double-release risk.Using only tag pushes plus workflow_dispatch removes duplicate runs from release-created triggers. LGTM.
13-16: Least-privilege default at top level — good.Job-scoped write permissions are added where needed. LGTM.
113-119: Artifact upload indentation fix retained — good.retention-days correctly sits under with:, not path. LGTM.
213-234: Retry loop is well-formed with explicit failure on exhaustion — nice.
290-311: Prod install retry loop mirrors TestPyPI improvements — looks solid.
145-150: Release-notes script present — workflow call verified.
scripts/extract_release_notes.py exists and contains anif __name__ == '__main__'entry (line 44), so the workflow'spython scripts/extract_release_notes.py $VERSIONinvocation is valid.
1 participant
Description
Improves the CI.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit