[linters] Install tools based on versions in package.json #2323

chalin · 2022-02-07T21:42:42Z

I'm having issues running certain spec checks from the website because it's working from a spec submodule. This PR:

Switches to using npx to run npm commands rather than hardcoded paths
Uses npm install to install all necessary NPM-based tools
- Adds a package.json file for this purpose -- with markdownlint-cli pinned to v0.31.0 as was done in Pin markdownlint-cli version and apply fixes #2320

IMHO, this is a clean and conventional way to deal with NPM packages. WDYT?

/cc @austinlparker

CONTRIBUTING.md

Makefile

package.json

yurishkuro

❤️

Makefile

joaopgrassi

This is great! I just left a few small things.

Makefile

package.json

Makefile

chalin · 2022-02-10T19:38:58Z

Rebased and conflicts resolved. Ready for a final review.

joaopgrassi

Looks good to me! I only have one concern about pinning package versions to avoid surprises (as we had a few days ago). But I'll leave that to others to consider as well.

.github/workflows/checks.yaml

package.json

chalin · 2022-02-11T14:31:37Z

@joaopgrassi - thanks for the feedback! I've made the changes that you suggested. PTAL

.github/workflows/checks.yaml

package.json

chalin · 2022-02-15T14:51:57Z

I believe that the last issue has been addressed. PTAL

joaopgrassi

LGTM and thanks for working on this!

github-actions · 2022-02-23T03:17:13Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

joaopgrassi · 2022-02-23T09:03:02Z

Hi @chalin, I think this is ready right? @anuraaga maybe you can take another look? Would be a pity if this is closed due to inactivity

CONTRIBUTING.md

Makefile

chalin · 2022-03-01T15:04:40Z

Hi all: I've restored the old target behavior by using install-checks. I see this as win-win, WDYT @Oberon00, @joaopgrassi, @yurishkuro @carlosalberto and others? While conceptually less clean, this addresses the concerns that @Oberon00 raised. My hope is that we can now have enough of a consensus to get this PR merged.

CONTRIBUTING.md

joaopgrassi

Both approaches are fine to me. I'm no expert in Make, but the build passes and I don't see anything clearly wrong. 👍

chalin · 2022-03-01T17:19:50Z

Thanks for the feedback @joaopgrassi.
Is this ready to merge, before the bot closes it!?! 😉 😄

yurishkuro · 2022-03-01T23:02:39Z

I am resolving discussions and merging. Feel free to open new tickets if changes are needed, but in the current state the PR is an overall improvement.

tigrannajaryan · 2022-03-03T20:30:31Z

@chalin After this change I can no longer do make markdownlint on my Mac. I get:

$ make markdownlint
/Users/tnajaryan/work/repos/opentelemetry-specification
└─┬ [email protected]
  └── [email protected] 

./CHANGELOG.md
npx: installed 203 in 6.874s
npx: command not found: markdownlint-cli
make: *** [markdownlint] Error 1

Any advice on what I need to do to make this work locally?

yurishkuro · 2022-03-03T20:41:24Z

Indeed - why did CI pass then?

It works for me after this patch:

diff --git a/Makefile b/Makefile
index e0d64e4..755e8e6 100644
--- a/Makefile
+++ b/Makefile
@@ -58,7 +58,7 @@ markdownlint:
        @if ! npm ls markdownlint; then npm install; fi
        @for f in $(ALL_DOCS); do \
                echo $$f; \
-               npx --no -p markdownlint-cli markdownlint -c .markdownlint.yaml $$f \
+               npx --no -p markdownlint markdownlint -c .markdownlint.yaml $$f \
                        || exit 1; \
        done

however it seems to be re-doing npm install for each file:

./specification/compatibility/opencensus.md
npx: installed 203 in 4.611s
./specification/compatibility/openmetrics.md
npx: installed 203 in 4.363s
./specification/compatibility/opentracing.md
npx: installed 203 in 4.437s
./specification/context/api-propagators.md
npx: installed 203 in 4.312s
./specification/context/context.md

yurishkuro · 2022-03-03T20:41:45Z

I suggest we revert this PR until it can be properly fixed

yurishkuro · 2022-03-03T20:43:31Z

also, we need to add .nvmrc that specifies the required Node version. I tried running with v10 and some installed packages didn't like it, they were >=12

chalin · 2022-03-03T21:19:49Z

Checking. Yes, adding an .nvmrc is a good idea. I can do that if you'd like.

chalin · 2022-03-03T21:31:15Z

Indeed, possibly a Node version issue.

@chalin After this change I can no longer do make markdownlint on my Mac. I get:
$ make markdownlint
/Users/tnajaryan/work/repos/opentelemetry-specification
└─┬ [email protected]
  └── [email protected] 

./CHANGELOG.md
npx: installed 203 in 6.874s
npx: command not found: markdownlint-cli
make: *** [markdownlint] Error 1
Any advice on what I need to do to make this work locally?

It works fine for me locally under macOS 11.6.4. I'm using the latest LTS version of Node:

$ nvm current
v16.13.0

You?

chalin · 2022-03-03T21:38:31Z

also, we need to add .nvmrc that specifies the required Node version. ...

Done, see #2401.

yurishkuro · 2022-03-03T21:40:28Z

Confirmed, works w/ v16, but not with v14.

tigrannajaryan · 2022-03-04T00:46:52Z

v16 works for me too.

…etry#2323) * [editorial] Use `npx` to run npm cmds, not hardcoded paths * Update Makefile based on feedback * Use --no-install option for npx * Add `npm install` to markdown-check workflows * Pin markdownlint-cli at 0.31.0 * Run `npm install` as a separate workflow step * Use name 'install dependencies' for npm install cmd * CONTRIBUTING: clarify that you install the tools once * make: add `install-tools` as dependency to `all` * Add install check line to `mardown*` targets Co-authored-by: Yuri Shkuro <[email protected]>

chalin requested review from a team February 7, 2022 21:42

github-actions bot assigned jmacd Feb 7, 2022

chalin commented Feb 7, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

chalin commented Feb 7, 2022

View reviewed changes

Makefile Show resolved Hide resolved

chalin commented Feb 7, 2022

View reviewed changes

package.json Show resolved Hide resolved

jmacd approved these changes Feb 7, 2022

View reviewed changes

yurishkuro approved these changes Feb 8, 2022

View reviewed changes

anuraaga reviewed Feb 8, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

joaopgrassi reviewed Feb 8, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

package.json Show resolved Hide resolved

chalin commented Feb 8, 2022

View reviewed changes

Makefile Show resolved Hide resolved

chalin force-pushed the chalin-use-npx-2022-02-07 branch from 030c93d to 33be543 Compare February 8, 2022 14:00

arminru added the editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. label Feb 9, 2022

chalin force-pushed the chalin-use-npx-2022-02-07 branch from 33be543 to 5ca87aa Compare February 10, 2022 19:34

joaopgrassi approved these changes Feb 11, 2022

View reviewed changes

.github/workflows/checks.yaml Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

joaopgrassi approved these changes Feb 11, 2022

View reviewed changes

.github/workflows/checks.yaml Outdated Show resolved Hide resolved

package.json Show resolved Hide resolved

yurishkuro mentioned this pull request Feb 15, 2022

Replace Python yamllint with NPM yaml-lint #2347

Open

chalin force-pushed the chalin-use-npx-2022-02-07 branch from 0fa49c7 to c354ac7 Compare February 15, 2022 19:37

joaopgrassi approved these changes Feb 15, 2022

View reviewed changes

github-actions bot added the Stale label Feb 23, 2022

Oberon00 reviewed Feb 23, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

chalin force-pushed the chalin-use-npx-2022-02-07 branch from c354ac7 to 0e63459 Compare February 23, 2022 15:37

yurishkuro reviewed Feb 23, 2022

View reviewed changes

Makefile Show resolved Hide resolved

yurishkuro approved these changes Feb 23, 2022

View reviewed changes

github-actions bot removed the Stale label Feb 24, 2022

chalin added 6 commits March 1, 2022 09:39

Add npm install to markdown-check workflows

152b2d8

Pin markdownlint-cli at 0.31.0

32f7185

Run npm install as a separate workflow step

ee4e6ff

Use name 'install dependencies' for npm install cmd

8eae1a1

CONTRIBUTING: clarify that you install the tools once

04c983a

make: add install-tools as dependency to all

7843f42

chalin force-pushed the chalin-use-npx-2022-02-07 branch from 21d4389 to 7843f42 Compare March 1, 2022 14:39

Add install check line to mardown* targets

780954e

joaopgrassi reviewed Mar 1, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

joaopgrassi approved these changes Mar 1, 2022

View reviewed changes

Oberon00 approved these changes Mar 1, 2022

View reviewed changes

Merge branch 'main' into chalin-use-npx-2022-02-07

7e480dd

yurishkuro changed the title ~~[editorial] Use npx to run npm cmds, not hardcoded paths~~ [linters] Install tools based on versions in package.json Mar 1, 2022

yurishkuro merged commit f6e07be into open-telemetry:main Mar 1, 2022

chalin deleted the chalin-use-npx-2022-02-07 branch March 2, 2022 00:20

chalin mentioned this pull request Mar 3, 2022

[linters] Recommend latest LTS version of Node #2401

Merged

chalin mentioned this pull request Mar 23, 2022

[linters] Check link-anchor for # stutter #2313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[linters] Install tools based on versions in package.json #2323

[linters] Install tools based on versions in package.json #2323

chalin commented Feb 7, 2022

yurishkuro left a comment

joaopgrassi left a comment

chalin commented Feb 10, 2022

joaopgrassi left a comment

chalin commented Feb 11, 2022

chalin commented Feb 15, 2022

joaopgrassi left a comment •

edited

Loading

github-actions bot commented Feb 23, 2022

joaopgrassi commented Feb 23, 2022

chalin commented Mar 1, 2022

joaopgrassi left a comment

chalin commented Mar 1, 2022

yurishkuro commented Mar 1, 2022

tigrannajaryan commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

chalin commented Mar 3, 2022

chalin commented Mar 3, 2022 •

edited

Loading

chalin commented Mar 3, 2022 •

edited

Loading

yurishkuro commented Mar 3, 2022

tigrannajaryan commented Mar 4, 2022

[linters] Install tools based on versions in package.json #2323

[linters] Install tools based on versions in package.json #2323

Conversation

chalin commented Feb 7, 2022

yurishkuro left a comment

Choose a reason for hiding this comment

joaopgrassi left a comment

Choose a reason for hiding this comment

chalin commented Feb 10, 2022

joaopgrassi left a comment

Choose a reason for hiding this comment

chalin commented Feb 11, 2022

chalin commented Feb 15, 2022

joaopgrassi left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2022

joaopgrassi commented Feb 23, 2022

chalin commented Mar 1, 2022

joaopgrassi left a comment

Choose a reason for hiding this comment

chalin commented Mar 1, 2022

yurishkuro commented Mar 1, 2022

tigrannajaryan commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

yurishkuro commented Mar 3, 2022

chalin commented Mar 3, 2022

chalin commented Mar 3, 2022 • edited Loading

chalin commented Mar 3, 2022 • edited Loading

yurishkuro commented Mar 3, 2022

tigrannajaryan commented Mar 4, 2022

joaopgrassi left a comment •

edited

Loading

chalin commented Mar 3, 2022 •

edited

Loading

chalin commented Mar 3, 2022 •

edited

Loading