-
-
Notifications
You must be signed in to change notification settings - Fork 241
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
Add npm-package-json-lint #2150
Add npm-package-json-lint #2150
Conversation
Some tests failed... @nvuillam do you know why this might be? I just added a new definition in a descriptor. |
You are missing test files + build ^^ |
@nvuillam I have created the tests and followed the instructions in that link but I keep getting failed tests. https://github.com/oxsecurity/megalinter/actions/runs/3747503607/jobs/6363797376 The problem is that it is not clear to me with the logs what I am doing wrong because in the logs it just tells you FAILED but it does not say what exactly is wrong. Is there any way to get more information? Looking at the assert I should see those messages, shouldn't I? Example:
Is there any way to run the tests locally? If I have to rely on github actions it takes like 45 minutes to run it. |
@bdovaz you can run the test cases locally yes I use VsCode + TestExplorer extension for that, you can even use breakpoints |
@nvuillam I am trying to run them locally but because I am on Windows I seem to fail several tests unrelated to this PR? There seems to be a discrepancy of \ vs /. Aren't these tests cross-platform? If they are not, what do I do? |
@bdovaz focus on the tests on new linter, if they work the others should work in CI :) |
@nvuillam But the problem is that I see discrepancies: https://github.com/oxsecurity/megalinter/actions/runs/3747503607/jobs/6363797376 This test works for me locally (on CI as you can see, it doesn't): FAILED megalinter/tests/test_megalinter/mega_linter_1_test.py::mega_linter_1_test::test_config_reporter This is a test that talks about JAVASCRIPT and I haven't touched any of that: FAILED megalinter/tests/test_megalinter/mega_linter_1_test.py::mega_linter_1_test::test_disable_linter As much as I focus on the ones that fail and show up in the job output, I see strange results. |
It seems that all the errors are because npm-package-json-lint command is not found
Package is npm-package-json-lint but CLI command seems to be npmPkgJsonLint (cf https://npmpackagejsonlint.org/docs/cli/) Case when the linter name is not the command name is handlable: to use command npmPkgJsonLint you must define |
@nvuillam I was going to try it but when doing merge against main it seems that a json file is broken, isn't it? megalinter-configuration.jsonschema.json |
@bdovaz sorry, conflict poorly managed by git + me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz check comments :)
@nvuillam I have it almost, only 2 tests of this PR and this linter in particular are failing. The problem is that npmPkgJsonLint it expects a folder path as parameter and not the full package.json file path. Look at the examples: https://npmpackagejsonlint.org/docs/cli#examples And that's why the test fails: Can you think how to solve it? |
Is there possibly a way to append |
@nvuillam can you think how? In this PR only a linter has been added to a descriptor, unless it can be solved at that level and it is with a property that I don't know, I don't know if it will be easy to solve this problem. |
@bdovaz i think you can try the following cli_lint_mode: project
cli_lint_extra_args_after: ["."] That was the call will be |
Ah, right, sorry, I should have said |
Codecov Report
@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
+ Coverage 82.84% 82.89% +0.04%
==========================================
Files 166 167 +1
Lines 4431 4437 +6
==========================================
+ Hits 3671 3678 +7
+ Misses 760 759 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@bdovaz you're almost there :) |
@nvuillam now it fails me in the Trivy scan, I understand that it is not related to my changes, no? |
@bdovaz this is a new vulnerability issue that has been discovered today, and as it is not present in the package you added, it is indeed not your fault ! |
@bdovaz as this is related to input and we don't input anything within MegaLinter, I consider we can ignore the vulnerability To do that, add CVE-2022-23529 in .trivyignore file at the root of the repo |
@bdovaz i did it in another PR, just make your branch up to date with |
@nvuillam It seems to be all right, doesn't it? |
@bdovaz It is ! :) To do that, you need to:
Please can you do that in another PR, as I just merged this one ? :) |
#2092