Skip to content
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

tools: apply linting to first commit in PRs #22452

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 22, 2018

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 Trott added the tools Issues and PRs related to the tools directory. label Aug 22, 2018
@Trott
Copy link
Member Author

Trott commented Aug 22, 2018

Aw yeah...

screen shot 2018-08-21 at 7 27 05 pm

Trott referenced this pull request Aug 22, 2018
`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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 22, 2018

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

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

This comment has been minimized.

@mscdex
Copy link
Contributor

mscdex commented Aug 22, 2018

Why only the first commit?

@Trott
Copy link
Member Author

Trott commented Aug 22, 2018

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

@mscdex
Copy link
Contributor

mscdex commented Aug 22, 2018

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

@Trott
Copy link
Member Author

Trott commented Aug 22, 2018

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.

@targos
Copy link
Member

targos commented Aug 22, 2018

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

@Trott
Copy link
Member Author

Trott commented Aug 22, 2018

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.

@Trott 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 vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Aug 22, 2018
@refack
Copy link
Contributor

refack commented Aug 22, 2018

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)

@targos
Copy link
Member

targos commented Aug 22, 2018

@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

.travis.yml Outdated
@@ -11,6 +14,8 @@ matrix:
- NODE=$(which node) make lint-md-build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

refack commented Aug 22, 2018

@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 🤦‍♂️

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

Any progress here?

@Trott
Copy link
Member Author

Trott commented Sep 9, 2018

Rebased/force-pushed to eliminate conflict.

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

@Trott Trott force-pushed the travis-updates branch 3 times, most recently from 7bf5e39 to 0b7466d Compare October 5, 2018 13:31
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.
jasnell pushed a commit that referenced this pull request Oct 17, 2018
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
refack added a commit to refack/node that referenced this pull request Nov 6, 2018
PR-URL: nodejs#23739
Fixes: nodejs#23737
Refs: nodejs#22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
targos pushed a commit that referenced this pull request Nov 6, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#23739
Fixes: nodejs#23737
Refs: nodejs#22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
@Trott Trott deleted the travis-updates branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.