tools: apply linting to first commit in PRs by Trott · Pull Request #22452 · nodejs/node · GitHub

First commit:

tools: only use Travis-CI on master branch

Limit Travis-CI to the master branch. One benefit is that it will
simplify the logic for commit message linting.

Second commit:

tools: apply linting to first commit in PRs

Use Travis-CI to check the formatting of the first commit in a pull
request. This will hopefully reduce formatting errors and nits about
them in pull requests.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/623/pipeline

Aw yeah...

`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion. PR-URL: #21906 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Ruby <rubys@intertwingly.net>

Since this alters .travis.yml and nothing else, the pipeline job and the Travis run should be sufficient rather than a full CI.

Why only the first commit?

Why only the first commit?

Because subsequent commits are often autosquash commits and I don't want to discourage people from using that. Maybe in the very near future, if we can create a CLI flag for core-validate-commit that skips commits that start with squash! or fixup!, we can lint all the commits. /cc @evanlucas @nodejs/automation

Is there no way to have CI try to auto squash all of the commits first?

Is there no way to have CI try to auto squash all of the commits first?

Oooh, I do like that idea! The --autosquash flag is only honored for interactive rebases. There's no doubt some way to make interactive mode not need to be interactive (such as using expect although there are probably even easier ways) but I'm not sure I want to go down that potentially gotcha-ridden rabbit hole before landing this. I'd prefer this land as it is now, and then pursue that as a subsequent enhancement.

I would like to avoid the first commit if possible. As someone who works a lot with canary and release branches, having Travis run on them is very useful

I would like to avoid the first commit if possible. As someone who works a lot with canary and release branches, having Travis run on them is very useful

OK. I wasn't aware that it was unusually valuable in that context. I'll see if I can figure something out after I sleep and attend the TSC meeting. :-D I'll remove the fast-track label and mark this as in progress. If someone knows how to make it work for everything, by all means, PR into the branch or commit right to it. Thanks.

I would like to avoid the first commit if possible. As someone who works a lot with canary and release branches, having Travis run on them is very useful

This has been enabled only for PR on master, and with --no-validate-metadata, it might actually be useful even for release and canary branches. It's validates only the common sense rules:

f918801e0fd1b89531e9230c985cadea91914a6e
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ✔  0:0      Title is <= 50 columns.                   title-length

Once we teach core-validate-commit to ignore commits that start with squash! or fixup! IMHO it could work for all commits (in PRs on master)

@refack my comment wasn't about core-validate-commit. What I care about is that Travis CI runs when commits are pushed to canary-base, v10.x-staging, v10.x, etc

Could you add a before_install: stage:

before_install: npm i -g core-validate-commit

instead of the npx call.
I find this more readable (one less magic word in L18)

@refack my comment wasn't about core-validate-commit. What I care about is that Travis CI runs when commits are pushed to canary-base, v10.x-staging, v10.x, etc

Ahh the first commit of this PR, not the first commit rule 🤦‍♂️

Any progress here?

Rebased/force-pushed to eliminate conflict.

Will get back to this soon, I think...

Use Travis-CI to check the formatting of the first commit in a pull request. This will hopefully reduce formatting errors and nits about them in pull requests.

Use Travis-CI to check the formatting of the first commit in a pull request. This will hopefully reduce formatting errors and nits about them in pull requests. PR-URL: #22452 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

PR-URL: nodejs#23739 Fixes: nodejs#23737 Refs: nodejs#22452 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>

PR-URL: #23739 Fixes: #23737 Refs: #22452 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>

PR-URL: nodejs#23739 Fixes: nodejs#23737 Refs: nodejs#22452 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>

Use Travis-CI to check the formatting of the first commit in a pull request. This will hopefully reduce formatting errors and nits about them in pull requests. PR-URL: #22452 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

PR-URL: #23739 Fixes: #23737 Refs: #22452 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>

Trott added the tools Issues and PRs related to the tools directory. label Aug 22, 2018

Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 22, 2018

This comment has been minimized.

Sign in to view

starkwang approved these changes Aug 22, 2018

View reviewed changes

trivikr approved these changes Aug 22, 2018

View reviewed changes

Trott added wip Issues and PRs that are still a work in progress. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Aug 22, 2018

vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Aug 22, 2018

refack reviewed Aug 22, 2018

View reviewed changes

Trott force-pushed the travis-updates branch from e8be106 to f47a05c Compare September 9, 2018 05:12

Trott force-pushed the travis-updates branch 3 times, most recently from 7bf5e39 to 0b7466d Compare October 5, 2018 13:31

tools: apply linting to first commit in PRs

43e129e

Use Travis-CI to check the formatting of the first commit in a pull request. This will hopefully reduce formatting errors and nits about them in pull requests.

Trott force-pushed the travis-updates branch from 0b7466d to 43e129e Compare October 5, 2018 13:48

refack mentioned this pull request Oct 18, 2018

build,meta: don't fail Travis for commit message #23739

Merged

3 tasks

richardlau mentioned this pull request Oct 19, 2018

tools: add script to lint commit messages #23758

Closed

1 task

richardlau mentioned this pull request Nov 9, 2018

lib: skip fixup! and squash! commits nodejs/core-validate-commit#33

Closed

MylesBorins added the land-on-v8.x label Nov 26, 2018

BethGriggs mentioned this pull request Dec 4, 2018

v8.14.1 proposal #24832

Merged

Trott deleted the travis-updates branch January 13, 2022 22:50

tools: apply linting to first commit in PRs by Trott · Pull Request #22452 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: apply linting to first commit in PRs - #22452

tools: apply linting to first commit in PRs#22452Trott wants to merge 1 commit intonodejs:masternodejs/node:masterfrom Trott:travis-updatesTrott/io.js:travis-updatesCopy head branch name to clipboard

Conversation

Trott commented Aug 22, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 22, 2018

Uh oh!

Trott commented Aug 22, 2018

Uh oh!

Trott commented Aug 22, 2018

Uh oh!

This comment has been minimized.

mscdex commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 22, 2018

Uh oh!

mscdex commented Aug 22, 2018

Uh oh!

Trott commented Aug 22, 2018

Uh oh!

targos commented Aug 22, 2018

Uh oh!

Trott commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Aug 22, 2018

Uh oh!

targos commented Aug 22, 2018

Uh oh!

refack Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 22, 2018

Uh oh!

BridgeAR commented Sep 5, 2018

Uh oh!

Trott commented Sep 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

tools: apply linting to first commit in PRs#22452
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:travis-updates

mscdex commented Aug 22, 2018 •
edited
Loading

Trott commented Aug 22, 2018 •
edited
Loading