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 support for Bicep Linter #1898

Merged
merged 32 commits into from
Oct 2, 2022
Merged

Add support for Bicep Linter #1898

merged 32 commits into from
Oct 2, 2022

Conversation

omusavi
Copy link
Contributor

@omusavi omusavi commented Sep 22, 2022

Bicep is a domain-specific language similar to HCL that allows for deploying Azure resources. The Bicep Linter validates Bicep files prior to them being transpiled to JSON.

This change aims to add the Bicep Linter to MegaLinter.

Fixes #1017

Proposed Changes

  1. Add support for Bicep to existing ARM linter rules

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

@nvuillam
Copy link
Member

nvuillam commented Sep 26, 2022

@omusavi i think you also need to put a level lower the file_contains_regex constraint, because your .bicep files are excluded because they does not contain such regex :)

[Filters] {'name': 'ARM_BICEP_LINTER', 'filter_regex_include': None, 'filter_regex_exclude': None, 'files_sub_directory': None, 'lint_all_files': False, 'lint_all_other_linters_files': False, 'file_extensions': ['.bicep'], 'file_names_regex': [], 'file_names_not_ends_with': [], 'file_contains_regex': ['schema\.management\.azure\.com']}

@nvuillam
Copy link
Member

image

@omusavi you're really close, your "bad" example only triggers a warning, so the linter returns 0 as exit code
If you add an error (or arguments ?) that will make the linter return any exit code > 0 when analyzing error file, you'll be all good :)

@omusavi
Copy link
Contributor Author

omusavi commented Sep 27, 2022

@nvuillam thanks for helping out with the debugging! Taking a look now...

@omusavi
Copy link
Contributor Author

omusavi commented Sep 27, 2022

@nvuillam actually have a question about how other linters have gotten around this...

Turns out that the CLI does not put out a non-zero exit code (nor does it have a flag for it). I will be submitting feedback to hopefully update that in the future, but for now this is what we have.

There is a way for people to specifically add a config file to the folder and the linter should pick that up, however since by default everything is a warning, people would need to know to set everything to errors in that config file. Do you recommend doing something like using cli_lint_errors_count to parse out the warnings as errors and have people set them to info in the case where they do not care to fix it, or should we defer to the linter settings and just have it be a warning by default?

If we go with the latter option, is it ok to check in a bicepconfig.json in the test folder with warnings as errors just for the sake of testing?

Thanks!

@nvuillam
Copy link
Member

@omusavi is it possible to use an argument explaining where to find this bicepconfig file ?

In lots of cases, MegaLinter provides a default configuration file (all files in this folder are just here for that -> https://github.com/oxsecurity/megalinter/tree/main/TEMPLATES )

The behaviour is the following:

  • if config file (config_file_name in descriptor) found at the root of the repo, do not use default one from MegaLinter
  • else use default one ( something like --config path/to/megalinterdefaults ) . -- config being cli_config_arg_name in descriptors )

@omusavi
Copy link
Contributor Author

omusavi commented Sep 27, 2022

@nvuillam There is no flag to point to the bicepconfig file (more feedback for the bicep linter team....), it will pick up whatever file is "closest" to the bicep files. In my testing just now, that seems to mean either co-located with the files, or any folder in the tree above the bicep files under test.

In this case, if there is a default one provided in the TEMPLATES folder, I am assuming that will not be present in a root directory of the workspace?

edit: Found this issue in the bicep github page: Azure/bicep#5013
Also this: Azure/bicep#2811

@nvuillam
Copy link
Member

In that case I think you can create the following structure (test classes will detect good and bad folders )

  • .automation/test/bicep
    • good
      • good_1.bicep
      • good_2.bicep
    • bad
      • bad_1.bicep
      • bad_2.bicep
      • bicepconfig.json

the _2 files are in case bicep can handle multiple files in one call ( else just put the _1 files )

You also need to define test_folder : bicep in the descriptor

@omusavi
Copy link
Contributor Author

omusavi commented Sep 28, 2022

Sounds good, will update with those changes

@omusavi
Copy link
Contributor Author

omusavi commented Sep 28, 2022

@nvuillam looks like our change to move file_contains_regex to the linter level was incorrect - that setting seems to be only at the root descriptor level and not on individual linters.

I guess at this point might make sense to create a new descriptor file for Bicep - even though its very closely related to ARM, they have different file formats so a lot of the shared things dont really match up. Will update that in addition to the test fixes...

@nvuillam
Copy link
Member

I probably forgot to take in account file_contains_regex at lower level ^^
We can either fix that or indeed create another descriptor:)

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2022

Codecov Report

Merging #1898 (60d3a62) into main (190f3d8) will increase coverage by 0.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1898      +/-   ##
==========================================
+ Coverage   82.00%   82.71%   +0.71%     
==========================================
  Files         156      157       +1     
  Lines        3378     3384       +6     
==========================================
+ Hits         2770     2799      +29     
+ Misses        608      585      -23     
Impacted Files Coverage Δ
...test_megalinter/linters/bicep_bicep_linter_test.py 100.00% <100.00%> (ø)
megalinter/MegaLinter.py 83.53% <0.00%> (+0.23%) ⬆️
...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
Copy link
Member

@omusavi it seems that you finally did it ,congrats 😄

Last updates you (optionally) may perform before removing the "draft" statut of the PR:

@nvuillam
Copy link
Member

  • sarif descriptor params if bicep is capable of SARIF output (i'm about to submit MegaLinter to default github code scanning github actions ^^ )

@omusavi
Copy link
Contributor Author

omusavi commented Sep 29, 2022

@nvuillam unfortunately no SARIF output, I think this is something we can create an issue about in the bicep repo. Other settings added! Thanks for all your help, this was quite a journey :D

@omusavi omusavi marked this pull request as ready for review September 29, 2022 13:33
@@ -5,18 +5,23 @@ descriptor_flavors:
linters:
- cli_help_arg_name: --help
cli_executable: bicep
cli_lint_mode: file
cli_lint_errors_count: "\\.bicep\\(\\d+,\\d+\\) : Error "
Copy link
Member

Choose a reason for hiding this comment

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

the properties are cli_lint_errors_count and cli_lint_errors_regex :)

Copy link
Member

Choose a reason for hiding this comment

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

cli_lint_error_count can be

  • regex_sum ( sum all the numbers found by regex in logs)
  • regex_number (find a single number with regex in logs)
  • regex_count (count the number of times the regex is matched in the log)

@nvuillam
Copy link
Member

@nvuillam unfortunately no SARIF output, I think this is something we can create an issue about in the bicep repo. Other settings added! Thanks for all your help, this was quite a journey :D

When there are error count regex, the test classes passes after >= 2 errors, just to make sure it can count 😁

I really think this is the last update I'll ask ^^

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.

Once CI it's ok it's all good, thanks a lot for your contribution :)
( @omusavi i allowed myself to add a last commit so bicep linter can be in today's planned released :) )

@nvuillam nvuillam merged commit c636075 into oxsecurity:main Oct 2, 2022
@omusavi
Copy link
Contributor Author

omusavi commented Oct 2, 2022

Thanks @nvuillam! Glad we got this in!

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.

Add linter support for bicep language
3 participants