Implement: Finalize default levels for linting rules

[StephenWeatherford]

Tracking subissues:

• Linters: ability to configure the "default" level for new analyzers#10304
• The linter ruleset should be versionable

Proposal - Change default levels for linting rules

Problem statement

While developing linter rules, we have deliberately enabled all new rules by default (almost all of them set to Warning by default). Now that we have a growing list of rules, it's time to set more appropriate defaults. Some should be off by default, other warning, and still others error. (It seems questionable that 'Info' is an appropriate default for any rule.)

Guiding principles

I believe the best way to determine which level each rule should default to is to consider the type/category of the rule. In the future, it might make sense to be able to easily set defaults for all rules in a category, or to allow importing a user-defined set of defaults. For now though, the categories are intended only for guidance and are not proposed to be an official part of anything in the linter or rules. (If official categories should be higher priority, please let us know.)

Warnings vs errors

Principal agreed on in community call is that if a triggered linter rule would definitely cause an actual deployment failure it should default to "error". Otherwise it should default to "warning".

Types of rules

I see the rules as falling into these approximate categories:

• Style
  • Style issues that cause no actual harm, encourage consistency or readability
  • E.g. no-unnecessary-dependson, prefer-unquoted-property-names
• Correctness
  • Issues that would cause actual errors during deployment (e.g. max-params)
  • E.g. max-params
• Best practice
  • Suggested best practices that may help avoid bugs or unexpected behavior
  • E.g. use-stable-resource-identifiers, artifacts-parameters
• Security
  • Possible security issues
• Portability
  • Issues that might affect usage of a bicep file by multiple users
  • E.g. no-hardcoded-env-urls

Again, for now these categories are just to help determine default level, nothing official.

Default level for each category

Category (unofficial)	Default level
Style	Warning
Portability	Off
Best Practice	Warning
Security	Warning
Deployment Error	Error

Default levels for current rules

Rule	Default level	Reason
adminusername-should-not-be-literal	error warning	Security
outputs-should-not-contain-secrets	error warning	Security (may have false positives)
protect-commandtoexecute-secrets	error warning	Security
secure-parameter-default	error warning	Security
secure-secrets-in-params	error warning	Security (may have false positives)
max-outputs	error	Error during deployment
max-params	error	Error during deployment
max-resources	error	Error during deployment
max-variables	error	Error during deployment
artifacts-parameters	warning	Best practice
no-unused-existing-resources	Warning	Best practice, might indicate bug from oversight
no-unused-params	Warning	Best practice, might indicate bug from oversight
no-unused-vars	Warning	Best practice, might indicate bug from oversight
use-stable-resource-identifiers	warning	Best practice/correctness
no-unnecessary-dependson	warning	Style
prefer-interpolation	warning	Style
prefer-unquoted-property-names	warning	Style
simplify-interpolation	warning	Style
no-hardcoded-env-urls	off	Portability (between clouds)
explicit-values-for-loc-params	Off warning (not likely to cause a lot of noise)	Portability Best practice/correctness (location) - Ensures an explicit value is passed in to modules' location parameters (not left as default)
no-hardcoded-location	Off warning (++see note)	Portability (authoring modules) Best practice (location)
no-loc-expr-outside-params	Off warning (++see note)	Portability (authoring modules) Best practice (location)
use-recent-api-version	off	arm-ttk parity (can't really be called best practice, but some work environments require it, as do QuickStarts) (CONSIDER: What's the best category?)
use-stable-vm-image	off	arm-ttk parity (CONSIDER: What's the best category?)

Note

These two we aleady know are controversial. I suggest they default to "warning" rather than "off" because:
1) They can help avoid bugs, for example:
a) one resource accidentally placed in a different region, especially with copy/paste, or
, as they generate a lot of noise
b) portability issue if some resources are not supported in the current resource group's location
2) They can easily be turned off in a bicepconfig.json file (or soon in a #disable-in-file pragma)

Documentation

CONSIDER: Document each rule's default level in online docs
CONSIDER: Document each rule's default value in its Intellisense description

Do we need an easy way to adjust default levels for rules?

Do we need the ability to:

1. change the default level of all rules in a category (e.g. turn off all style rules)?
2. turn all rules on or off by default?
3. change all warnings to errors?

In particular, would the following be useful?

{
  "analyzers": {
    "core": {
      "enabled": true,
      "defaultLevel": "warning",    // << PROPOSED (changes all rules to default to "warning", can be overridden at rule level)
      "warningsAsErrors": true,  // << PROPOSED (changes all "warning" rules to "error", can be overridden at rule level)
      "rules": {
        ...
      }
    }
  }
}

[afscrome]

For warningsAsErrors, I'd find it more useful to be able to set this as a command line argument on build rather than in a config file. For local dev, warnings as errors can be painful. (Maybe you've temporarily commented out a resource, but don't want to go and fix all the other params relating to that resource). But on a build server, I'd want to enforce that all warnings are resolved (unless explicitly supressed).

I currently have this implemented by having our azure devops task fail if anything is written to stdout, but this has proven problematic before (See #3638) so a native method on the build command would be preferred.

[jtracey93]

Nice work @StephenWeatherford

I'd really like to see the ability to set the level for all rules in a category easily in one go from either the command line, as suggested above, and also in the bicepconfig.json file; and say the command line wins/takes preference (following parameter value input logic today for deployment).

If this is enabled I think warnings as errors can be removed/not implemented as the flexibility will be there if enabled in both command line and bicepconfig.json files.

Maybe if there was a keyword lookup for something like "allCategories" as well this provides a single input option to configure individual categories or all in one go. Instead of a conflicting additional option to configure.

• On another note, do you see configurable input values for some rules to be set like "recent api versions"? E.g. where can I set the timespan that I deem "recent"? Or will this be hard coded and un-configurable?

Thanks

Jack

[slavizh]

As stated before for me rule no-loc-expr-outside-params should be off by default. It just generates a lot of noise. Even when you use for example resourceGroup().location in module name or on variable you get a warning. Also it does not fits well in best practices as usually if the resource is not global it is recommended that your resources are in the same location as the location of the resource group - https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/best-practices#resource-group. The rule is even triggered when you use deployment().location. I really do not get why this rule exists at all.

[AllainPL]

My biggest issue with no-loc-expr-outside-params is that from an exploratory/learning standpoint

1. you enter the world of bicep and you want nice modular code
2. you think you have a relatively sensible piece of code location: resourceGroup().location
3. you get the warning, so you move it to param location string = resourceGroup().location
4. you are happy you have no more warnings - just to realize in a different file you just got explicit-values-for-loc-params
5. so now you fix all your code adding a lot of verbosity with locations while you really want to put everything just into one resourceGroup().location ;)

So, unless there is a sensible pattern here for something like a "global location" those warnings feel like a loop leading to more verbosity for some users.

And I'm not sure if I am really against this, it just feels like we are somehow running circles here :) @majastrz nicely summarized it in #6210

Edit: the globals idea was also mentioned here #399 (comment) by @anthony-c-martin

[alex-frankel]

@StephenWeatherford -- do we need more discussion on this one? Otherwise, can I remove the "discussion" label?

[StephenWeatherford]

Removed "discussion" for now.

[StephenWeatherford]

• On another note, do you see configurable input values for some rules to be set like "recent api versions"? E.g. where can I set the timespan that I deem "recent"? Or will this be hard coded and un-configurable?

@jtracey93 I've asked about this in the past and seen little interest. It wouldn't be hard to add, if you think it should be configurable, please enter an issue so we can gauge interest.

[StephenWeatherford]

Changed: Location rules moved to (theoretical) portability category, will be turned off by default.

[StephenWeatherford]

Updated due to community call: Security rules will default to "warning", not "error".