Conversation
…process. Updated pull request trigger types and improved the logic for determining the reference commit during merges. Enhanced error handling for rebase operations in the version increment step.
… improving error handling during version increments. Streamlined logic for determining reference commits and optimized rebase operations.
…s 24 for JavaScript actions
There was a problem hiding this comment.
PR Summary:
- Removes
synchronizefrom PR trigger events (open PRs no longer re-run CI on new commits) - Simplifies the
refexpression in the checkout step - Adds
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24env var andshell: bash+set -eto the version bump step - Adds a brand-new
docs/ci-cd.mddocumenting the entire CI/CD flow, registered inmkdocs.yml
Review Summary:
The documentation addition is thorough and well-structured, using correct MkDocs !!! admonition syntax as required by the repo's MkDocs/Material theme setup. The workflow hardening (explicit bash shell, set -e, informative rebase error message) is a solid improvement.
One notable concern: the simplified ref expression on the checkout step removed the merged guard that was present in the original. For opened/reopened events GitHub still populates merge_commit_sha with a test-merge ref — but it can be null during the async computation window right after a PR is opened, and always null for PRs with merge conflicts. This means conflicting or freshly opened PRs may fall back to github.sha (the base branch tip) rather than the PR head, silently checking out the wrong commit. Restoring the merged-guard or switching to head.sha for non-merge events would make this robust.
The deliberate removal of synchronize is flagged as a medium concern — it's a meaningful UX change (no CI re-run on subsequent pushes to an open PR) that is documented in the new CI/CD doc but worth a conscious team sign-off.
Suggestions
- Add
synchronizeback to PR trigger types but scope it to the path filter, so CI re-runs on new pushes to open PRs without affecting merge-triggered WIP/Release logic. Apply - Replace the hardcoded
developbranch URL inci-cd.mdwith a permalink to the current commit SHA to prevent link drift after releases. Apply
⚡ Apply all quick fixes (2 quick fixes)
| fetch-depth: 0 | ||
| submodules: recursive | ||
| ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.merged && github.event.pull_request.merge_commit_sha != '' && github.event.pull_request.merge_commit_sha || github.sha }} | ||
| ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.merge_commit_sha || github.sha }} |
There was a problem hiding this comment.
The simplified ref expression removes the merged guard, so for opened/reopened events it will attempt to check out merge_commit_sha — GitHub's test merge ref — rather than the actual PR head commit. This has two risks:
- Race condition on
opened/reopened: GitHub computes the test merge commit asynchronously. If the workflow fires before that computation completes (common with freshly opened PRs),merge_commit_shaisnull, and the expression short-circuits togithub.sha(the base branch tip), checking out entirely the wrong commit. - Conflict PRs: If the PR has merge conflicts,
merge_commit_shaisnullfor the same reason.
For a non-merge CI run the intended target should be the PR branch tip (github.event.pull_request.head.sha), not the test merge commit. The previous guard ensured merge_commit_sha was only used post-merge. Suggested fix:
| ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.merge_commit_sha || github.sha }} | |
| ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.merged && github.event.pull_request.merge_commit_sha != '' && github.event.pull_request.merge_commit_sha || github.sha }} |
Or more robustly, use head.sha for open PRs:
ref: ${{ github.event_name == 'pull_request' && (github.event.pull_request.merged && github.event.pull_request.merge_commit_sha || github.event.pull_request.head.sha) || github.sha }}actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| # PR: opened / reopened / closed (merged close runs WIP or Release). Pushes to the PR branch do not trigger (no synchronize). | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, closed] | ||
| types: [opened, reopened, closed] |
There was a problem hiding this comment.
Removing synchronize means commits pushed to an open PR no longer trigger CI. This is intentional per the comment, but it has a functional impact: the path filter paths: [...] is evaluated against the diff of the current push/PR. For opened/reopened events the diff comparison base is the PR base branch; for closed (merge) it's irrelevant since CI must run for WIP/Release regardless.
The practical problem: if a contributor opens a PR that initially only touches docs (skipped by path filter), then pushes a commit touching pyrevitlib/ — there is now no way for CI to run on that PR without reopening it. The doc comment acknowledges "Close and reopen the PR" as the workaround, but this is a significant UX regression from the original synchronize behavior. Consider whether this trade-off is acceptable and if the documentation warning is sufficient.
Additionally: workflow_dispatch has no paths filter, so it's always available as a manual escape hatch, which partially mitigates this.
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
|
||
| ## Workflow: `pyRevitCI` | ||
|
|
||
| The pipeline lives in [`.github/workflows/main.yml`](https://github.com/pyrevitlabs/pyRevit/blob/develop/.github/workflows/main.yml) as job **`pyRevitCI`**. |
There was a problem hiding this comment.
The hardcoded link points to the develop branch even though this file lives in master-targeted docs:
[`.github/workflows/main.yml`](https://github.com/pyrevitlabs/pyRevit/blob/develop/.github/workflows/main.yml)
Consider using a branch-agnostic permalink (commit SHA or default branch reference) or omit the branch qualifier entirely so the link doesn't drift out of date after master diverges from develop.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
📦 New public release are available for 6.3.0.26091+2238 |
updated workflow and docs (CI/CD)