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

[Feature Request]: Handle new Bicep linter warning prefer-unquoted-property-names #1664

Closed
AlexanderSehr opened this issue Jul 12, 2022 · 6 comments · Fixed by #2200
Closed
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Jul 12, 2022

Description

Recently, a new Bicep linter warning prefer-unquoted-property-names was introduced which leads to a large amount of warnings regarding our modules.

The primary reason are our nested RBAC modules (nested_roleAssignments.bicep) as they contain a map of RoleName - RoleID mappings, and the RoleName is always formatted as a string.

image

This was originally done to make the list easy to read (i.e., consistent formatting), but we may want to revisit that decision now.

For the time being, with PR #1632, the warning is disabled. If we want to enable it again, we should make sure all our modules align with the new rule.

cc: @itpropro

@rahalan
Copy link
Contributor

rahalan commented Aug 18, 2022

Team decided not to follow the linter recommendation. To still not be bothered by this warning, a bicep config file needs to be set up.
TODO: This should also be mentioned in the "known issues".

@rahalan rahalan added documentation Improvements or additions to documentation and removed [cat] needs further discussion labels Aug 18, 2022
@itpropro
Copy link
Contributor

itpropro commented Sep 8, 2022

Team decided not to follow the linter recommendation. To still not be bothered by this warning, a bicep config file needs to be set up. TODO: This should also be mentioned in the "known issues".

Hi @rahalan,
thanks for the update, can you please add the reasoning for deviating from the official bicep team recommendations here?
Declaring separate linter configurations in this repository makes it more difficult for customers to integrate the CARML library, as they normally have their own linter configurations already and from my experienced with bicep projects, the prefer-unquoted-property-names is used for internal linting a lot. So an official reasoning, why they have to configure the bicep linting in another way than recommended by the bicep team is necessary to be able to integrate CARML modules in the future.

@AlexanderSehr
Copy link
Contributor Author

AlexanderSehr commented Sep 30, 2022

Hey @itpropro,
even though I'd support the motion, a quorum decided to leave it as is for because a mix of quoted and unquoted strings in the list appears to confuse more than it helps & the generating utility would need to be updated too.

In the bicepconfig.json it says

{
    "analyzers": {
        "core": {
            "rules": {
                (..)
                "prefer-unquoted-property-names": {
                    "level": "off" // Reason: This complains primarily about RBAC roles which are all in quotes to be consistent within the list of roles with and without spaces in their name
                }
            }
        }
    }
}

However, @eriqua provided the alternative suggestion to a ignore rules in the RBAC files instead. This would mean 4/5 lines per file, but we could keep the linter rule for everyhing else active - as it is a good rule.

@AlexanderSehr
Copy link
Contributor Author

Attached a Draft PR for reference

@itpropro
Copy link
Contributor

itpropro commented Oct 3, 2022

Hey @itpropro, even though I'd support the motion, a quorum decided to leave it as is for because a mix of quoted and unquoted strings in the list appears to confuse more than it helps & the generating utility would need to be updated too.

In the bicepconfig.json it says

{
    "analyzers": {
        "core": {
            "rules": {
                (..)
                "prefer-unquoted-property-names": {
                    "level": "off" // Reason: This complains primarily about RBAC roles which are all in quotes to be consistent within the list of roles with and without spaces in their name
                }
            }
        }
    }
}

However, @eriqua provided the alternative suggestion to a ignore rules in the RBAC files instead. This would mean 4/5 lines per file, but we could keep the linter rule for everyhing else active - as it is a good rule.

I think the suggestion from @eriqua for a ignore rules in the RBAC files is a good compromise. This way, customers are still able to use their own linter rules and can keep their linter configs intact without getting warnings thrown.

@rahalan
Copy link
Contributor

rahalan commented Oct 13, 2022

Team decides to go with the rule and remove quotes on single strings.
Team decides to go for having all roles in alphabetical order, regardless which one has quotes, see https://github.com/Azure/ResourceModules/pull/2200/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment