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

doc, tools: add doc linting to CI #12640

Closed
wants to merge 1 commit into from
Closed

doc, tools: add doc linting to CI #12640

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
Affected core subsystem(s)

doc, tools

Fixes: #12635
Refs: #12563

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 25, 2017
@vsemozhetbyt vsemozhetbyt added the doc Issues and PRs related to the documentations. label Apr 25, 2017
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

Just 1 Windows machine unstable in CI.

For a test, I shall add an error in doc and run a new CI.

@vsemozhetbyt
Copy link
Contributor Author

CI that SHOULD FAIL: https://ci.nodejs.org/job/node-test-pull-request/7666/

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

Whohoo! Linter fails.

Do I get it right that linter in CI is run only as a separate job and is not run in builds? So we can test only Makefile here and not vcbuild.bat?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

It seems so, as only the linter job fails. Reverting the error...

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

Please, let me know if this can be landed without delay as a follow-up fix for #12563. Currently, we have a conflict between local and CI linting. We can land commits that will break ESLint runs in local buildings.

cc @not-an-aardvark, @silverwind, @Trott

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

The changes LGTM, but I'll defer to others about whether this should be landed without waiting 48 hours.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@gibfahn
Copy link
Member

gibfahn commented Apr 25, 2017

+1 to landing sooner to avoid CI/local linting issues

@vsemozhetbyt
Copy link
Contributor Author

I will dare to land now having got a one +1 and some LGTM.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 42dca99

@vsemozhetbyt vsemozhetbyt deleted the doc-lint-ci branch April 25, 2017 19:47
vsemozhetbyt added a commit that referenced this pull request Apr 25, 2017
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

It seems all the old PRs will still use the old linting in CI if not rebased.

The same example: #12549 (comment)

New linter CI was launched after landing this PR, but console log shows old linting commands: https://ci.nodejs.org/job/node-test-linter/8559/console

I think this is an additional reason for having this landed sooner.

@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

Will be backported to v6.x with #12563 if that lands.

@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* Install [email protected]
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* Install [email protected]
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
* Install [email protected]
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: #12563
Refs: #12640
Refs: #14047

PR-URL: #14067
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants