-
Notifications
You must be signed in to change notification settings - Fork 43
feat: run linters and tests on each push, not just on open PRs #1622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run: | | ||
| BASE=$(git merge-base ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }}) | ||
| BASE=$(git merge-base ${{ github.sha }} ${{ github.sha }}) | ||
| pnpx commitlint \ | ||
| --from "$BASE" \ | ||
| --to ${{ github.event.pull_request.head.sha }} | ||
| --to ${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint range collapses to a single commit on push
Because BASE is computed as git merge-base ${{ github.sha }} ${{ github.sha }}, the base always equals the current SHA, so --from and --to are identical. On a push that contains multiple commits (e.g., after a rebase or when pushing a series of local commits), this configuration only lints the tip commit and skips earlier commits, allowing invalid commit messages through. If the intent is to validate all commits introduced by the push, use the previous head (e.g., github.event.before) as the start of the range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That's legit. Marking this as do-not-merge for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
This compares github.sha with itself, so git merge-base will return the same commit.
caio-pizzol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to shift to push-based validation (note: this will increase CI usage - which I think is OK in this case)
| run: | | ||
| BASE=$(git merge-base ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }}) | ||
| BASE=$(git merge-base ${{ github.sha }} ${{ github.sha }}) | ||
| pnpx commitlint \ | ||
| --from "$BASE" \ | ||
| --to ${{ github.event.pull_request.head.sha }} | ||
| --to ${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
This compares github.sha with itself, so git merge-base will return the same commit.
Rename GitHub workflow from
pr-validationtocommit-validation, and run on all pushes to all branches, rather than only running for open PRs.Concurrency is configured to cancel earlier builds on branches except for main.