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

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

Merged
merged 6 commits into from
Jan 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ Note: Can be used with `oxsecurity/megalinter@beta` in your GitHub Action mega-l
- Change name of config file for powershell formatter to avoid collision with powershell linter config
- Enhance find SARIF json in stdout output
- Pass --show-context, --show-suggestions, and --no-must-find-files to CSpell for friendlier UX
by @Kurt-von-Laven in [#2271](https://github.com/oxsecurity/megalinter/issues/2271).
by @Kurt-von-Laven in [#2275](https://github.com/oxsecurity/megalinter/pull/2275).
- Only run npm-package-json-lint when package.json is present by @Kurt-von-Laven in
[#2280](https://github.com/oxsecurity/megalinter/pull/2280).

- Documentation
- Configure jsonschema documentation formatting (see [Descriptor schema](https://megalinter.io/latest/json-schemas/descriptor.html), [Configuration schema](https://megalinter.io/latest/json-schemas/configuration.html)), by @echoix in [#2270](https://github.com/oxsecurity/megalinter/pull/2270)
Expand Down
2 changes: 2 additions & 0 deletions megalinter/descriptors/json.megalinter-descriptor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ linters:
- salesforce
file_names_regex:
- "package\\.json"
active_only_if_file_found:
- "package.json"
cli_lint_mode: project
cli_executable: npmPkgJsonLint
cli_config_arg_name: --configFile
Expand Down
58 changes: 24 additions & 34 deletions megalinter/flavor_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,31 +84,31 @@ def check_active_linters_match_flavor(active_linters):
missing_linters = []
for active_linter in active_linters:
if active_linter.name not in flavor_linters:
missing_linters += [active_linter.name]
active_linter.is_active = False
# 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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

nah that's ok ^^

Copy link
Member

@nvuillam nvuillam Jan 31, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

missing_linters_str = ",".join(missing_linters)
logging.warning(
f"MegaLinter flavor [{flavor}] does not contain linters {missing_linters_str}.\n"
"As they are not available in this docker image, they will not be processed\n"
"To solve this problem, please either: \n"
f"- use default flavor {ML_REPO}\n"
"- add ignored linters in DISABLE or DISABLE_LINTERS variables in your .mega-linter.yml config file "
"located in your root directory\n"
"- ignore this message by setting config variable FLAVOR_SUGGESTIONS to false"
)
# Stop the process if user wanted so in case of missing linters
if config.get("FAIL_IF_MISSING_LINTER_IN_FLAVOR", "") == "true":
logging.error(
'Missing linter and FAIL_IF_MISSING_LINTER_IN_FLAVOR has been set to "true": Stop run'
)
sys.exit(84)
return False
# All good !
return True
# Ignore linters that shouldn't trigger failure when missing.
if not active_linter.name.startswith("REPOSITORY"):
missing_linters.append(active_linter.name)

if not missing_linters:
return True

missing_linters_str = ",".join(missing_linters)
logging.warning(
f"MegaLinter flavor [{flavor}] does not contain linters {missing_linters_str}.\n"
"As they are not available in this Docker image, they will not be processed\n"
"To solve this problem, please either: \n"
f"- use default flavor {ML_REPO}\n"
"- add ignored linters in DISABLE or DISABLE_LINTERS variables in your .mega-linter.yml config file "
"located in your root directory\n"
"- ignore this message by setting config variable FLAVOR_SUGGESTIONS to false"
)
# Stop the process if user wanted so in case of missing linters
if config.get("FAIL_IF_MISSING_LINTER_IN_FLAVOR", "") == "true":
logging.error(
'Missing linter and FAIL_IF_MISSING_LINTER_IN_FLAVOR has been set to "true": Stop run'
)
sys.exit(84)
return False


# Compare active linters with available flavors to make suggestions to improve CI performances
Expand Down Expand Up @@ -147,13 +147,3 @@ def get_megalinter_flavor_suggestions(active_linters):
)
new_flavor_linters_names = map(lambda linter: linter.name, new_flavor_linters)
return ["new", new_flavor_linters_names]


def are_all_repository_linters(linter_names: list[str]) -> bool:
if len(linter_names) == 0:
return False
result = True
for linter_name in linter_names:
if not linter_name.startswith("REPOSITORY"):
result = False
return result