ci: run only fast tests and skip slow notebooks on draft PRs#562
Conversation
📝 WalkthroughWalkthroughGitHub Actions documentation and test workflows now monitor workflow-file changes, use explicit pull request event types, execute slow notebooks for eligible documentation builds, and exclude selected test categories for draft pull requests. ChangesCI workflow execution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
FBumann
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docs.yaml (1)
87-93: Consider aligning draft-check syntax with tests.yaml.The condition uses GitHub Actions expression syntax (
!= true), whiletests.yamlline 76 uses bash string comparison (== "true"). Both work correctly, but for consistency and explicitness, consider adopting the bash approach used in tests.yaml:- - name: Execute slow notebooks - if: steps.notebook-cache.outputs.cache-hit != 'true' && github.event.pull_request.draft != true + - name: Execute slow notebooks + if: steps.notebook-cache.outputs.cache-hit != 'true' && github.event.pull_request.draft != 'true'Note: Sequential execution (via
cat | xargs) is appropriate for slow notebooks to avoid resource contention, unlike the parallel execution (xargs -P 4) used for fast notebooks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/docs.yaml.github/workflows/tests.yaml
🔇 Additional comments (3)
.github/workflows/tests.yaml (2)
6-10: LGTM: Path filters enhance workflow precision.The path filters appropriately trigger the workflow for relevant changes (source code, tests, dependencies, and workflow configs). Including
.github/workflows/**ensures the CI re-runs when the workflow definitions themselves change.Also applies to: 13-17
75-82: The review comment is based on an incorrect assumption.The pytest configuration in
pyproject.tomldoes not containaddoptssettings that exclude examples. The configuration only defines the three markers (slow,examples,deprecated_api) without any default exclusion rules.In the workflow:
- Draft PRs: Examples are explicitly excluded via the
-m "not examples and not slow and not deprecated_api"flag in the pytest command- Non-draft/push: Pytest runs without any marker filters, meaning examples will run
The comment on line 80 stating "examples excluded via addopts" is misleading. Examples are not excluded by default configuration; they're only excluded for draft PRs via the explicit
-mflag.Likely an incorrect or invalid review comment.
.github/workflows/docs.yaml (1)
10-10: LGTM: Path filters aligned with tests workflow.Adding
.github/workflows/**andflixopt/**ensures documentation builds when workflow configs or source code change, maintaining consistency with the tests workflow.Also applies to: 15-16
- flixopt/** (source code) - tests/** (test files) - pyproject.toml (config/dependencies)
…onfig changes will trigger both.
5c97458 to
ee49258
Compare
FBumann
changed the title
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/docs.yaml (1)
52-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials in the CI jobs that run repository-controlled code.
actions/checkoutpersists credentials by default. Setpersist-credentials: falseat.github/workflows/docs.yaml#L52-L54and eachactions/checkoutstep in.github/workflows/tests.yaml; leave the deploy checkout authenticated because it pushes withmike.🤖 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/docs.yaml around lines 52 - 54, Disable persisted checkout credentials by adding persist-credentials: false to the actions/checkout steps at .github/workflows/docs.yaml lines 52-54 and .github/workflows/tests.yaml lines 35, 60, 91, and 112. Leave the deploy checkout that uses mike unchanged because it requires authentication to push.Source: Linters/SAST tools
🤖 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/docs.yaml:
- Around line 97-103: Update the notebook cache configuration associated with
the “Execute slow notebooks” step so draft and ready-for-review/full runs cannot
share a cache entry containing different notebook outputs. Include the execution
mode and slow_notebooks.txt in the cache key, or disable cache saving for draft
runs, while preserving the existing slow-notebook skip behavior on cache hits.
In @.github/workflows/tests.yaml:
- Around line 75-82: Add a workflow-level permissions block granting only
contents: read, alongside the existing workflow configuration in tests.yaml.
Keep the pytest execution branches unchanged and ensure no broader token scopes
are introduced.
---
Outside diff comments:
In @.github/workflows/docs.yaml:
- Around line 52-54: Disable persisted checkout credentials by adding
persist-credentials: false to the actions/checkout steps at
.github/workflows/docs.yaml lines 52-54 and .github/workflows/tests.yaml lines
35, 60, 91, and 112. Leave the deploy checkout that uses mike unchanged because
it requires authentication to push.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: be757317-434c-405e-b012-608aa00a57cc
📒 Files selected for processing (2)
.github/workflows/docs.yaml.github/workflows/tests.yaml
| - name: Execute slow notebooks | ||
| if: steps.notebook-cache.outputs.cache-hit != 'true' && github.event.pull_request.draft != 'true' | ||
| run: | | ||
| set -eo pipefail | ||
| # Execute slow notebooks in parallel (skip on draft PRs) | ||
| cd docs/notebooks && cat slow_notebooks.txt | \ | ||
| xargs -P 4 -I {} sh -c 'jupyter execute --inplace "$1" || exit 255' _ {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .github/workflows/docs.yaml
wc -l .github/workflows/docs.yaml
sed -n '1,220p' .github/workflows/docs.yamlRepository: flixOpt/flixopt
Length of output: 6367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect cache-related references in the workflow and nearby docs notebook paths.
rg -n "notebook-cache|slow_notebooks|ready_for_review|draft != 'true'|actions/cache|cache-key|cache-hit" .github/workflows/docs.yaml docs -SRepository: flixOpt/flixopt
Length of output: 1563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the workflow references separate cache keys or restore-keys, inspect the exact expressions.
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/docs.yaml')
text = p.read_text()
for needle in ['notebook-cache', 'slow_notebooks', 'ready_for_review', 'cache-hit', 'actions/cache', 'restore-keys', 'key:']:
print(f'## {needle}')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: flixOpt/flixopt
Length of output: 1670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' .github/workflows/docs.yamlRepository: flixOpt/flixopt
Length of output: 6307
Separate draft and full notebook caches. The cache key only hashes notebooks and flixopt source, so a draft PR run can save fast-only outputs under the same key that a later ready_for_review run will hit and then skip the slow notebooks. Include execution mode/slow_notebooks.txt in the key, or prevent draft runs from saving the cache.
🤖 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/docs.yaml around lines 97 - 103, Update the notebook cache
configuration associated with the “Execute slow notebooks” step so draft and
ready-for-review/full runs cannot share a cache entry containing different
notebook outputs. Include the execution mode and slow_notebooks.txt in the cache
key, or disable cache saving for draft runs, while preserving the existing
slow-notebook skip behavior on cache hits.
| run: | | ||
| if [[ "${{ github.event.pull_request.draft }}" == "true" ]]; then | ||
| # Draft PR: skip examples, slow, and deprecated_api | ||
| pytest -v --numprocesses=auto -m "not examples and not slow and not deprecated_api" | ||
| else | ||
| # Ready PR & main push: examples excluded via addopts | ||
| pytest -v --numprocesses=auto | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' .github/workflows/tests.yamlRepository: flixOpt/flixopt
Length of output: 3440
🌐 Web query:
GitHub Actions GITHUB_TOKEN permissions default contents read upload-artifact permissions required actions/upload-artifact checkout least privilege workflow permissions
💡 Result:
The GitHub Actions GITHUB_TOKEN is a temporary, automatically generated secret used to authenticate API requests within workflows [1][2]. Applying the principle of least privilege is a critical security best practice for GitHub Actions [3][4]. Default Permissions For most new repositories and organizations, the default permission for the GITHUB_TOKEN is set to read-only (specifically for contents and packages) [5][6][2]. You can verify or change this default in your repository or organization settings under Actions > General > Workflow permissions [5][2]. Permissions for Standard Actions 1. actions/checkout: This action typically requires read access to repository contents to fetch the code [1][6]. 2. actions/upload-artifact: This action does not rely on the GITHUB_TOKEN for its core functionality; it uses a separate, internal token managed by the GitHub Actions runner [7]. Therefore, you do not need to grant explicit GITHUB_TOKEN permissions for artifact uploads [7]. Note that "permission denied" errors encountered with this action are almost always related to file system permissions on the runner (e.g., trying to access a file created by a different user, such as root in Docker) rather than GITHUB_TOKEN scopes [8][9][10]. Implementing Least Privilege To enforce the principle of least privilege, explicitly define permissions in your workflow YAML file [3][4]. Any permission not listed under the permissions key is set to none by default [1]. Example of a minimal permissions configuration: permissions: contents: read If a specific job requires more access (e.g., to create an issue), you can scope the permissions for that job specifically: jobs: job_name: permissions: contents: read issues: write steps: - uses: actions/checkout@v4 - run: gh issue create... For complex workflows, you can use monitoring tools to identify the minimum set of permissions required before locking them down [2].
Citations:
- 1: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 2: https://github.blog/security/new-tool-to-secure-your-github-actions/
- 3: https://docs.github.com/actions/reference/authentication-in-a-workflow
- 4: https://docs.github.com/en/actions/reference/security/secure-use
- 5: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 6: https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
- 7: Add details on authentication / permissions used during upload actions/upload-artifact#197
- 8: https://github.com/actions/upload-artifact
- 9: https://github.com/actions/upload-artifact?tab=readme-ov-file
- 10: https://stackoverflow.com/questions/69977574/github-actions-upload-artifact-access-denied-trying-to-upload-a-file-despite
Restrict the workflow token here. Add a permissions block with contents: read at the workflow level; none of these jobs need additional scopes.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 51-82: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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/tests.yaml around lines 75 - 82, Add a workflow-level
permissions block granting only contents: read, alongside the existing workflow
configuration in tests.yaml. Keep the pytest execution branches unchanged and
ensure no broader token scopes are introduced.
Source: Linters/SAST tools
1 participant
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit