Conversation
Reviewer's GuideUpdates the GitHub Actions test workflow to inject a GFP API key into the environment and add a dedicated job that installs dependencies and runs the File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider scoping
GFP_API_KEYto just thetest_gdsfactoryplusjob (or even a single step) instead of defining it at the workflow level, to minimize exposure of the secret. - The
matrixdefinition intest_gdsfactoryplusonly has one Python version and one OS; if you don't plan to expand it, you can simplify by inliningpython-version: 3.12andruns-on: ubuntu-latestwithout a matrix. - If
test_gdsfactoryplusdepends on the other test jobs completing successfully, consider adding aneeds:clause so its execution order is explicit and future changes to the workflow don't accidentally change behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `GFP_API_KEY` to just the `test_gdsfactoryplus` job (or even a single step) instead of defining it at the workflow level, to minimize exposure of the secret.
- The `matrix` definition in `test_gdsfactoryplus` only has one Python version and one OS; if you don't plan to expand it, you can simplify by inlining `python-version: 3.12` and `runs-on: ubuntu-latest` without a matrix.
- If `test_gdsfactoryplus` depends on the other test jobs completing successfully, consider adding a `needs:` clause so its execution order is explicit and future changes to the workflow don't accidentally change behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Updated Python version handling and improved workflow steps.
9135993 to
77f20c9
Compare
77f20c9 to
01ac5fd
Compare
CI Workflow ReviewSecret scope too broad
Job will fail on fork PRs / missing secret When the secret is not configured (fork PRs, new contributors), test-gdsfactoryplus:
env:
GFP_API_KEY: ${{ secrets.GFP_API_KEY }}
if: env.GFP_API_KEY != ''Note: GitHub does not allow direct secret references in Missing install step
|
Review notesMissing result aggregation for the new matrix job The existing Without this, branch protection rules that require a specific named check to pass will see a per-shard set of statuses rather than one conclusive result. Consider adding a |
|
@joamatab @flaport Have you encountered the following error? Oddly enough this doesn't happen with macOS + Python 3.12, all others fail. |
Co-authored-by: nikosavola <7860886+nikosavola@users.noreply.github.com>
|
Now It's failing on 3.13 but not 3.12 |
Update gfp test branch
| [tool.gdsfactoryplus] | ||
| name = "qpdk" | ||
|
|
||
| [tool.gdsfactoryplus] |
There was a problem hiding this comment.
Duplicate [tool.gdsfactoryplus] table header. TOML does not allow the same table to be defined more than once — this is a spec violation and will cause parsers (e.g. tomllib, tombi) to error or silently drop one of the entries.
The original [tool.gdsfactoryplus] section already exists at line 65. The duplicate block here should be removed; all keys (name, drc, sim.x, pdk) belong to a single [tool.gdsfactoryplus] table and its sub-tables, which are already present below.
| [tool.gdsfactoryplus] |
Summary by Sourcery
Add CI coverage for the gdsfactoryplus (gfp) test command in the GitHub Actions workflow.
CI:
uv run gfp teston Python 3.12.