Only Run npm-package-json-lint When `package.json` is Present #2280

Kurt-von-Laven · 2023-01-20T13:03:20Z

Fixes #2279.

Proposed Changes

Only run npm-package-json-lint when package.json is present.
Don't include REPOSITORY linters in warning about missing linters.
Make minor syntactic simplifications in directly related code.

Readiness Checklist

Author/Contributor

Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

Label as breaking if this is a large fundamental change
Label as either automation, bug, documentation, enhancement, infrastructure, or performance

codecov-commenter · 2023-01-21T00:39:07Z

Codecov Report

Merging #2280 (c481644) into main (cc77d0c) will increase coverage by 0.63%.
The diff coverage is 20.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2280      +/-   ##
==========================================
+ Coverage   82.39%   83.02%   +0.63%     
==========================================
  Files         171      171              
  Lines        4521     4513       -8     
==========================================
+ Hits         3725     3747      +22     
+ Misses        796      766      -30

Impacted Files	Coverage Δ
megalinter/flavor_factory.py	`77.46% <20.00%> (+6.57%)`	⬆️
megalinter/reporters/UpdatedSourcesReporter.py	`89.74% <0.00%> (+2.56%)`	⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py	`89.01% <0.00%> (+8.05%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nvuillam · 2023-01-21T00:40:42Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	7		0	0.02s
✅ BASH	shellcheck	7		0	0.41s
✅ BASH	shfmt	7	0	0	0.04s
✅ COPYPASTE	jscpd	yes		no	2.61s
✅ DOCKERFILE	hadolint	105		0	8.19s
✅ JSON	eslint-plugin-jsonc	21	0	0	1.85s
✅ JSON	jsonlint	19		0	0.21s
✅ JSON	npm-package-json-lint	yes		no	0.62s
✅ JSON	v8r	21		0	14.42s
⚠️ MARKDOWN	markdownlint	309	2	229	6.09s
✅ MARKDOWN	markdown-link-check	309		0	5.22s
✅ MARKDOWN	markdown-table-formatter	309	2	0	17.24s
✅ OPENAPI	spectral	1		0	0.94s
⚠️ PYTHON	bandit	176		45	2.47s
✅ PYTHON	black	176	0	0	3.8s
✅ PYTHON	flake8	176		0	1.87s
✅ PYTHON	isort	176	0	0	0.45s
✅ PYTHON	mypy	176		0	7.08s
✅ PYTHON	pylint	176		0	12.02s
⚠️ PYTHON	pyright	176		280	18.74s
✅ REPOSITORY	checkov	yes		no	27.16s
⚠️ REPOSITORY	devskim	yes		61	1.23s
✅ REPOSITORY	dustilock	yes		no	1.6s
✅ REPOSITORY	git_diff	yes		no	0.04s
✅ REPOSITORY	secretlint	yes		no	5.09s
✅ REPOSITORY	syft	yes		no	3.25s
✅ REPOSITORY	trivy	yes		no	18.08s
✅ SPELL	cspell	730		0	18.91s
✅ SPELL	misspell	551	2	0	0.51s
✅ XML	xmllint	3	0	0	0.03s
✅ YAML	prettier	81	0	0	2.55s
✅ YAML	v8r	23		0	55.97s
✅ YAML	yamllint	82		0	1.47s

See detailed report in MegaLinter reports

You could have same capabilities but better runtime performances if you request a new MegaLinter flavor.

Click here to request the new flavor

MegaLinter is graciously provided by

nvuillam

Nice one :) to finalize it, plz check comment, + merge main into your branch :)

nvuillam · 2023-01-24T22:21:51Z

megalinter/flavor_factory.py

-    # Manage cases where linters are missing in flavor
-    if len(missing_linters) > 0:
-        # Do not warn/stop if missing linters are repository ones (mostly OX.security related)
-        if not are_all_repository_linters(missing_linters):


@Kurt-von-Laven plz could you rename are_all_repository_linters into are_ignorable_linters, and move your check inside the method ? thanks :)

The reason I moved the check up here was that I didn't think we should be including the repository linters in missing_linters since we display its contents to the user. I can decompose the check (and rename the helper method) and retain the effect of the commit copied below, but it feels indirect to add the linters into this list only to then immediately remove them. I could alternatively revert the commit copied below (and rename the helper method) if you would rather we continue listing these linters as missing.

Don't warn that REPOSITORY linters are missing

Some REPOSITORY linters are intentionally omitted from flavors to keep their size small and performance fast. Hence, their absence from a flavor isn't considered an error when FAIL_IF_MISSING_LINTER_IN_FLAVOR is true. Therefore, omit them from the list of missing linters displayed to the user to avoid confusion.

nah that's ok ^^

@Kurt-von-Laven I think you broke something here :(
Now the beta version of python flavor crashes before it looks for devskim, which is not in this flavor on purpose :/

Can you link me to or paste the stack trace or error message? All of the checks that I see passed when this PR was merged to main, so I'm guessing this is an issue that snuck past CI?

But the latest one works... I'm kind of lost, maybe it was just a version of beta that was wrongly built >_<
https://github.com/oxsecurity/megalinter/actions/runs/4052028152/jobs/6970984591

Ah, yes, I remember seeing that error inconsistently while working on this pull request and I believe on #2275 as well, so I assumed it was unrelated to either. It sounds like this may be a job for git bisect since we aren't sure when it started, although it probably would have to be scripted to try several times since it's not reproducing consistently.

It's back :(
https://github.com/oxsecurity/megalinter/actions/runs/4052519085/jobs/6972038673

It impacts mega-linter-runner tests, which use javascript flavor
https://github.com/oxsecurity/megalinter/actions/workflows/mega-linter-for-runner.yml

If I were to cd mega-linter-runner once and then npm run test repeatedly at each Git bisection point, would that be likely to isolate the issue to a particular commit?

nvuillam · 2023-01-24T22:23:51Z

@Kurt-von-Laven Another thought... why not just adding the following in the descriptor ?

active_only_if_file_found:
  - "package.json"

Kurt-von-Laven

Oh neat! Thanks for teaching me about active_only_if_file_found. Yes, I'll do that instead of exempting npm-package-json-lint from the missing linter list then.

Kurt-von-Laven · 2023-01-26T08:15:39Z

megalinter/flavor_factory.py

-    # Manage cases where linters are missing in flavor
-    if len(missing_linters) > 0:
-        # Do not warn/stop if missing linters are repository ones (mostly OX.security related)
-        if not are_all_repository_linters(missing_linters):


The reason I moved the check up here was that I didn't think we should be including the repository linters in missing_linters since we display its contents to the user. I can decompose the check (and rename the helper method) and retain the effect of the commit copied below, but it feels indirect to add the linters into this list only to then immediately remove them. I could alternatively revert the commit copied below (and rename the helper method) if you would rather we continue listing these linters as missing.

Don't warn that REPOSITORY linters are missing

Some REPOSITORY linters are intentionally omitted from flavors to keep their size small and performance fast. Hence, their absence from a flavor isn't considered an error when FAIL_IF_MISSING_LINTER_IN_FLAVOR is true. Therefore, omit them from the list of missing linters displayed to the user to avoid confusion.

The former is slightly faster as it generally doesn't create a new list.

Some REPOSITORY linters are intentionally omitted from flavors to keep their size small and performance fast. Hence, their absence from a flavor isn't considered an error when FAIL_IF_MISSING_LINTER_IN_FLAVOR is true. Therefore, omit them from the list of missing linters displayed to the user to avoid confusion.

Reduce indentation by inverting test, and prefer the more Pythonic test not list to len(list) == 0 for simplicity. Remove some comments rendered unnecessary via this more direct expression of our intent.

npm-package-json-lint only lints Node.js package.json files, so only run it when package.json is present. npm-package-json-lint is correctly omitted from most flavors. Many non-Node.js projects contain other JSON files, so this change prevents false positives when FAIL_IF_MISSING_LINTER_IN_FLAVOR is true.

nvuillam · 2023-01-26T08:34:47Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	7		0	0.02s
✅ BASH	shellcheck	7		0	0.43s
✅ BASH	shfmt	7	0	0	0.38s
✅ COPYPASTE	jscpd	yes		no	3.24s
✅ DOCKERFILE	hadolint	105		0	10.97s
✅ JSON	eslint-plugin-jsonc	21	0	0	2.67s
✅ JSON	jsonlint	19		0	0.31s
✅ JSON	v8r	21		0	16.6s
⚠️ MARKDOWN	markdownlint	309	0	229	7.57s
✅ MARKDOWN	markdown-link-check	309		0	5.97s
✅ MARKDOWN	markdown-table-formatter	309	0	0	19.4s
✅ OPENAPI	spectral	1		0	0.91s
⚠️ PYTHON	bandit	176		45	2.78s
✅ PYTHON	black	176	0	0	5.66s
✅ PYTHON	flake8	176		0	2.5s
✅ PYTHON	isort	176	0	0	0.83s
✅ PYTHON	mypy	176		0	9.07s
✅ PYTHON	pylint	176		0	15.55s
⚠️ PYTHON	pyright	176		282	22.82s
✅ REPOSITORY	checkov	yes		no	32.39s
✅ REPOSITORY	git_diff	yes		no	0.39s
✅ REPOSITORY	secretlint	yes		no	8.29s
✅ REPOSITORY	trivy	yes		no	29.35s
✅ SPELL	cspell	730		0	27.3s
✅ SPELL	misspell	551	0	0	1.0s
✅ XML	xmllint	3	0	0	0.45s
✅ YAML	prettier	81	0	0	3.41s
✅ YAML	v8r	23		0	62.65s
✅ YAML	yamllint	82		0	1.7s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

Kurt-von-Laven · 2023-01-27T04:01:11Z

The test failures seem unrelated to this PR.

nvuillam · 2023-01-27T15:12:03Z

Unauthentified calls to api.github.com limits reached , as usual :/

Let's run the again :)

nvuillam

Good catch ! :)

nvuillam · 2023-01-29T16:26:53Z

megalinter/flavor_factory.py

-    # Manage cases where linters are missing in flavor
-    if len(missing_linters) > 0:
-        # Do not warn/stop if missing linters are repository ones (mostly OX.security related)
-        if not are_all_repository_linters(missing_linters):


nah that's ok ^^

Kurt-von-Laven added the bug Something isn't working label Jan 20, 2023

Kurt-von-Laven requested a review from nvuillam as a code owner January 20, 2023 13:03

nvuillam requested changes Jan 24, 2023

View reviewed changes

Kurt-von-Laven commented Jan 26, 2023

View reviewed changes

Kurt-von-Laven added 6 commits January 26, 2023 00:18

Link to a PR rather than an issue in changelog

13caf7e

Prefer list.append(x) to list += [x]

c3db5ba

The former is slightly faster as it generally doesn't create a new list.

Simplify check_active_linters_match_flavor

b17027f

Reduce indentation by inverting test, and prefer the more Pythonic test not list to len(list) == 0 for simplicity. Remove some comments rendered unnecessary via this more direct expression of our intent.

Correct capitalization of Docker in a warning

903b07d

Kurt-von-Laven force-pushed the missing-linter branch from 069d738 to c481644 Compare January 26, 2023 08:30

Kurt-von-Laven changed the title ~~Don't Treat npm-package-json-lint as Missing From Flavors~~ Only Run npm-package-json-lint When package.json is Present Jan 26, 2023

nvuillam approved these changes Jan 29, 2023

View reviewed changes

nvuillam merged commit 40631ba into main Jan 29, 2023

nvuillam deleted the missing-linter branch January 29, 2023 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only Run npm-package-json-lint When `package.json` is Present #2280

Only Run npm-package-json-lint When `package.json` is Present #2280

Kurt-von-Laven commented Jan 20, 2023 •

edited

Loading

codecov-commenter commented Jan 21, 2023 •

edited

Loading

nvuillam commented Jan 21, 2023 •

edited

Loading

nvuillam left a comment

nvuillam Jan 24, 2023

Kurt-von-Laven Jan 26, 2023

nvuillam Jan 29, 2023

nvuillam Jan 31, 2023 •

edited

Loading

Kurt-von-Laven Jan 31, 2023

nvuillam Jan 31, 2023

Kurt-von-Laven Jan 31, 2023

nvuillam Jan 31, 2023

nvuillam Jan 31, 2023

Kurt-von-Laven Jan 31, 2023

nvuillam commented Jan 24, 2023 •

edited

Loading

Kurt-von-Laven left a comment

Kurt-von-Laven Jan 26, 2023

nvuillam commented Jan 26, 2023 •

edited

Loading

Kurt-von-Laven commented Jan 27, 2023

nvuillam commented Jan 27, 2023

nvuillam left a comment

nvuillam Jan 29, 2023

Only Run npm-package-json-lint When package.json is Present #2280

Only Run npm-package-json-lint When package.json is Present #2280

Conversation

Kurt-von-Laven commented Jan 20, 2023 • edited Loading

Proposed Changes

Readiness Checklist

Author/Contributor

Reviewing Maintainer

codecov-commenter commented Jan 21, 2023 • edited Loading

Codecov Report

nvuillam commented Jan 21, 2023 • edited Loading

🦙 MegaLinter status: ⚠️ WARNING

nvuillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvuillam Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvuillam commented Jan 24, 2023 • edited Loading

Kurt-von-Laven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvuillam commented Jan 26, 2023 • edited Loading

🦙 MegaLinter status: ⚠️ WARNING

Kurt-von-Laven commented Jan 27, 2023

nvuillam commented Jan 27, 2023

nvuillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Only Run npm-package-json-lint When `package.json` is Present #2280

Only Run npm-package-json-lint When `package.json` is Present #2280

Kurt-von-Laven commented Jan 20, 2023 •

edited

Loading

codecov-commenter commented Jan 21, 2023 •

edited

Loading

nvuillam commented Jan 21, 2023 •

edited

Loading

nvuillam Jan 31, 2023 •

edited

Loading

nvuillam commented Jan 24, 2023 •

edited

Loading

nvuillam commented Jan 26, 2023 •

edited

Loading