Skip to content

tools: only report commit validation failure on Slack #61124

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Dec 21, 2025
Merged

tools: only report commit validation failure on Slack #61124

nodejs-github-bot merged 1 commit into from
Dec 21, 2025

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 19, 2025

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2025

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Dec 19, 2025
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2025
@avivkeller
Copy link
Member

avivkeller commented Dec 19, 2025

Instead use using the failure() function, why not add continue-on-error: true to the step that can expectedly fail?

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 19, 2025

Instead use using the failure() function, why not add continue-on-error: true to the step that can expectedly fail?

What would be the upside of that vs what this PR is doing?

@avivkeller
Copy link
Member

avivkeller commented Dec 19, 2025

What would be the upside of that vs what this PR is doing?

Using it will make the check appear as successful. Without continue-on-error, the check would be marked as failed even though it actually succeeded (as the Slack notification was sent successfully).

failure() steps always run regardless of the workflow’s success or failure, whereas continue-on-error allows a step to fail without marking the workflow as failed.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 19, 2025 •
edited
Loading

One goal of #61050 was to mark the workflow as failure when commit message is invalid (same logic as linter failure shows as a failure)

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2025
@nodejs-github-bot nodejs-github-bot merged commit 607a741 into nodejs:main Dec 21, 2025
26 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2025

Landed in 607a741

@aduh95 aduh95 deleted the notify-on-push branch December 21, 2025 10:50
MatricalDefunkt pushed a commit to MatricalDefunkt/node that referenced this pull request Dec 22, 2025
@aduh95 @MatricalDefunkt
tools: only report commit validation failure on Slack
b86c5da 
PR-URL: nodejs#61124
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@meixg meixg meixg approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aduh95 @nodejs-github-bot @avivkeller @lpinca @cjihrig @meixg