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

Add npm-package-json-lint #2150

Merged
merged 20 commits into from
Dec 22, 2022

Conversation

bdovaz
Copy link
Collaborator

@bdovaz bdovaz commented Dec 20, 2022

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 20, 2022

Some tests failed... @nvuillam do you know why this might be? I just added a new definition in a descriptor.

@nvuillam
Copy link
Member

You are missing test files + build ^^

https://megalinter.io/beta/contributing/#add-a-new-linter

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

You are missing test files + build ^^

https://megalinter.io/beta/contributing/#add-a-new-linter

@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:

"Output IDE config file " + expected_output_file + " should exist",

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.

@nvuillam
Copy link
Member

@bdovaz you can run the test cases locally yes

I use VsCode + TestExplorer extension for that, you can even use breakpoints

image

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

@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?

image

@nvuillam
Copy link
Member

@bdovaz focus on the tests on new linter, if they work the others should work in CI :)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

@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.

@nvuillam
Copy link
Member

It seems that all the errors are because npm-package-json-lint command is not found

FileNotFoundError: [Errno 2] No such file or directory: 'npm-package-json-lint'

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 cli_executable: npmPkgJsonLint in the descriptor

@bdovaz bdovaz mentioned this pull request Dec 21, 2022
@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

@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

image

@nvuillam
Copy link
Member

@bdovaz sorry, conflict poorly managed by git + me
It's fixed now

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

plz check comments :)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

@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:

image

Can you think how to solve it?

@Kurt-von-Laven
Copy link
Collaborator

Is there possibly a way to append /.. to the end of the file path? Sometimes that can be less work than removing the filename.

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 21, 2022

Is there possibly a way to append /.. to the end of the file path? Sometimes that can be less work than removing the filename.

@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.

@nvuillam
Copy link
Member

@bdovaz i think you can try the following

cli_lint_mode: project
cli_lint_extra_args_after: ["."]

That was the call will be npmPkgJsonLint . or npmPkgJsonLint --config npmGroovyLintConfig.json .

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Dec 22, 2022

Ah, right, sorry, I should have said /., not /...

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022

Codecov Report

Merging #2150 (096035f) into main (2e0e163) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
...alinter/linters/json_npm_package_json_lint_test.py 100.00% <100.00%> (ø)
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️

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

@nvuillam
Copy link
Member

@bdovaz you're almost there :)
Just fix the dummy package.json at the root to make it appear valid and it will be ok :)
https://github.com/oxsecurity/megalinter/blob/main/package.json

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 22, 2022

@nvuillam now it fails me in the Trivy scan, I understand that it is not related to my changes, no?

@nvuillam
Copy link
Member

@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 !

https://avd.aquasec.com/nvd/2022/cve-2022-23529/

@nvuillam
Copy link
Member

@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

@nvuillam
Copy link
Member

@bdovaz i did it in another PR, just make your branch up to date with main and it will be ok :)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 22, 2022

@nvuillam It seems to be all right, doesn't it?

@nvuillam
Copy link
Member

nvuillam commented Dec 22, 2022

@bdovaz It is ! :)
I reviewed a last time, and I think this should not be in all MegaLinter flavors, but only the ones that may contain a package.json ( MegaLinter flavors are size-optimized to contain only linters related to some types of repos )

To do that, you need to:

  • override descriptor_flavors
  • remove the all_flavors and add the flavors that can contain a package.json
  • I suggest cupcake, javascript & salesforce, but maybe u'll see more
  • bash build.sh again

Please can you do that in another PR, as I just merged this one ? :)

@nvuillam nvuillam merged commit abfc7df into oxsecurity:main Dec 22, 2022
@bdovaz bdovaz deleted the feature/npm-package-json-lint branch December 22, 2022 22:09
@nvuillam nvuillam mentioned this pull request Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants