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.

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

build: do not lint fixtures in make lint-md #22549

Closed
wants to merge 1 commit into from
Closed

build: do not lint fixtures in make lint-md #22549

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

This is useful if we need to put markdown files in test/fixtures (I was working on pulling the WPT there)

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 27, 2018
@Trott
Copy link
Member

Trott commented Aug 28, 2018

I would prefer no changes to line 1093 and adding -not -path 'test/fixtures/*' to line 1096.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2018
@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

Removing author-ready because I kinda think addressing @Trott’s comment is worth it and this isn’t really all that urgent (I think)

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

Ping @joyeecheung

@joyeecheung
Copy link
Member Author

Updated to use the -not option instead.

CI: https://ci.nodejs.org/job/node-test-pull-request/17205/

@Trott
Copy link
Member

Trott commented Sep 15, 2018 •
edited by refack
Loading

Everything is good on CI except a known Windows flake. Windows re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20733/ (Update: ✔️)

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 15, 2018
@refack
Copy link
Contributor

refack commented Sep 15, 2018

Resume CI (with a special Windows trick): https://ci.nodejs.org/job/node-test-commit/21481/

@joyeecheung
Copy link
Member Author

Windows is still pretty red..

@refack
Copy link
Contributor

refack commented Sep 16, 2018

So again: https://ci.nodejs.org/job/node-test-pull-request/17219/

@Trott
Copy link
Member

Trott commented Sep 17, 2018

Only red was Windows fanned. Known flaky. Here's a re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20753/

@addaleax
Copy link
Member

… and again: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20782/

@joyeecheung
Copy link
Member Author

Windows build is (finally) green

@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 17, 2018 •
edited
Loading

Landed in bb9d788, thanks!

joyeecheung added a commit that referenced this pull request Sep 17, 2018
@joyeecheung
build: do not lint fixtures in make lint-md
bb9d788 
PR-URL: #22549
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Sep 18, 2018
@joyeecheung @targos
build: do not lint fixtures in make lint-md
6f99252 
PR-URL: #22549
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos targos mentioned this pull request Sep 18, 2018
Merged
targos pushed a commit that referenced this pull request Sep 19, 2018
@joyeecheung @targos
build: do not lint fixtures in make lint-md
e07e573 
PR-URL: #22549
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
targos pushed a commit that referenced this pull request Sep 20, 2018
@joyeecheung @targos
build: do not lint fixtures in make lint-md
3fcd54f 
PR-URL: #22549
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@refack refack refack 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. build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@joyeecheung @nodejs-github-bot @Trott @addaleax @BridgeAR @refack @danbev @jasnell