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

Check _RULES_PATH for active_only_if_file_found check #418

Merged
merged 11 commits into from May 20, 2021
Merged

Check _RULES_PATH for active_only_if_file_found check #418

merged 11 commits into from May 20, 2021

Conversation

omusavi
Copy link
Contributor

@omusavi omusavi commented Apr 18, 2021

Currently, active_only_if_file_found only checks the root workspace folder for the required files. These files may not always be present at the root, especially in a monorepo scenario. By looking in the _RULES_PATH folder, we can enable these linters to run as expected.

Fixes #374

Proposed Changes

  1. Respect LINTER_RULES_PATH when checking for required files to turn on certain linters (ESLint)
  2. When setting config_file and linter_rules_path is set, make it relative to self.workspace (might only be required for local execution where CWD is /)

Readiness Checklist

Author/Contributor

  • 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

@omusavi
Copy link
Contributor Author

omusavi commented Apr 18, 2021

First contribution, saw an issue that I am also running into so wanted to try and submit a fix! My concern is mostly around the 2nd commit where i am appending the linter_rules_path to workspace. It seems like a safe change to me (I am guessing linter rules are always relative to the workspace root?). I think this also only affects local runs since CWD is set differently when run with mega-linter-runner vs in github actions.

Marked as draft to make sure this is ok before merging

@omusavi
Copy link
Contributor Author

omusavi commented Apr 18, 2021

Also looks like the linter is failing in a file I did not modify, should I fix as a part of this PR or is it not something to worry about?

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

Merging #418 (7501427) into master (2026a05) will increase coverage by 0.93%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   86.70%   87.64%   +0.93%     
==========================================
  Files         126      126              
  Lines        2934     2937       +3     
==========================================
+ Hits         2544     2574      +30     
+ Misses        390      363      -27     
Impacted Files Coverage Δ
megalinter/Linter.py 89.10% <100.00%> (-0.59%) ⬇️
megalinter/MegaLinter.py 90.65% <0.00%> (+2.42%) ⬆️
megalinter/reporters/UpdatedSourcesReporter.py 89.18% <0.00%> (+2.70%) ⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py 99.12% <0.00%> (+9.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2026a05...7501427. Read the comment docs.

@nvuillam
Copy link
Member

nvuillam commented Apr 23, 2021

Sorry for the delay, i'll have a look asap :)

@nvuillam
Copy link
Member

Please can you try to merge master in your branch ?

  • add your update in root changelog.md
  • bash build.sh

Thanks :)

@omusavi
Copy link
Contributor Author

omusavi commented Apr 26, 2021

@nvuillam Updated! build.sh did not generate any new files to update. Assuming you feel this change is safe, I am setting it to ready for review

@omusavi omusavi marked this pull request as ready for review April 26, 2021 16:37
@omusavi omusavi requested a review from nvuillam as a code owner April 26, 2021 16:37
@nvuillam
Copy link
Member

nvuillam commented Apr 26, 2021

@omusavi CI is not happy... i think I know why :)
Default rules can be found in .github/linters, and you replaced the statement, whereas you should add another elif (before the one you replaced, so default rules are the last to be checked ) :)

image

@omusavi
Copy link
Contributor Author

omusavi commented Apr 28, 2021

Sorry I am not sure I understand what you mean. I don't think I replaced any checks, in the code in the image above I just prepended the workspace path to the linter rules path which is a workaround to get it working locally.

I guess I am a bit confused about the CWD in these cases (also called out in #419), are we expecting .github/linters to be in the workspace root?

@nvuillam
Copy link
Member

nvuillam commented May 15, 2021

@omusavi yes github/linters must be in workspace root :)

Basically, let the elif in red like it was before, and add an elif before with the code in green, and that should do the trick :)

image

@omusavi
Copy link
Contributor Author

omusavi commented May 19, 2021

Ok I still do not completed understand under what scenarios the file at self.linter_rules_path + os.path.sep + self.config_file_name exists but self.workspace + os.path.sep + self.linter_rules_path + os.path.sep + self.config_file_name doesn't if linter_fules_path is always inside self.workspace...perhaps absolute vs relative path then?

Added the change as you directed and it looks like CI is happy, so this is just for my own knowledge!

@@ -326,7 +329,8 @@ def load_config_vars(self):
# 1: http rules path: fetch remove file and copy it locally (then delete it after linting)
# 2: repo + config_file_name
# 3: linter_rules_path + config_file_name
# 4: mega-linter default rules path + config_file_name
# 4: workspace root + linter_rules_path + config_file_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to confirm this is the correct interpretation here...

@nvuillam
Copy link
Member

Ok I still do not completed understand under what scenarios the file at self.linter_rules_path + os.path.sep + self.config_file_name exists but self.workspace + os.path.sep + self.linter_rules_path + os.path.sep + self.config_file_name doesn't if linter_fules_path is always inside self.workspace...perhaps absolute vs relative path then?

Added the change as you directed and it looks like CI is happy, so this is just for my own knowledge!

Probably in MegaLinter own test classes :)

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.

Great, thanks for your contribution :)

@nvuillam nvuillam merged commit b26e6c4 into oxsecurity:master May 20, 2021
@omusavi omusavi deleted the check_rules_path_active_file branch June 3, 2021 19:05
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.

Difficulty using linters that define 'active_only_if_file_found' inside a monorepo
3 participants