Disable rules with inline comments

[sd65]

Hi all,

As a linter used in IDEs, the possibility to add inline comments to ignore a line would be useful.
I think this could inpire us.

What do you think?

[kddejong]

So far we have tried to treat json and yaml files the same but we wouldn't be able to do that in this case.

Technically I think this is possible. We would have to rework the yaml parser to store comments into an object we can parse and look for certain patterns, etc, etc.

It would be a little inefficient but we would have to capture all the failures and then see if each error was inside the enable/disable blocks.

Let me think on this... I'm working through a few more issues before I would be able to get to this one.

[sd65]

I see your point. I like your idea of capturing all errors and checking then if we can ignore them.

Furthermore, a first step would be to simply check for end of the line ignore rules comment. Then, the support for ignore block could come.

[adamchainz]

You can disable rules with the Metadata section, example in tests.

[kddejong]

Resources also support Metadata and we could add support to disable from there too. Then it would only disable for that resource. Sadly this doesn't help for other types of rules but may also be a decent alternative.

[ossek]

Is it possible to disable specific instances of check? E.g., since transform include is not supported (#15 (comment)) it would be nice to disable the type of output below (on a template that passes aws cloudformation validate-template). I still would like to see other instances of E3001 though.

E3001 Type not defined for resource Fn::Transform
shared_nested_stack_client/parent_template.yaml:18:3

E3006 Resources Fn::Transform has invalid name.  Name has to be alphanumeric.
shared_nested_stack_client/parent_template.yaml:18:3

E3001 Invalid resource attribute Name for resource Fn::Transform
shared_nested_stack_client/parent_template.yaml:19:5

E3001 Invalid resource attribute Parameters for resource Fn::Transform
shared_nested_stack_client/parent_template.yaml:20:5

[cmmeyer]

Right now you can only turn on and off rules. But for your use case, it seems like it might make more sense for us to enhance the linter to lint around Fn::Transform? I can add that as an enhancement.

[ossek]

Excellent, thanks @cmmeyer, let me know if I can provide any other information, and if you could add a link here for the issue once created, much appreciated.

[chizou]

I would like to chime in that I would also prefer to disable specific instances of a check.

E2520 is checking for mutual exclusions which is fine except when I'm also checking it in my template.

The below resource will throw E2520 because I'm specifying MasterUserPassword/MasterUsername with SnapshotIdentifier

  AuroraCluster:
    Properties:
      BackupRetentionPeriod: !Ref 'BackupRetention'
      DBClusterIdentifier: !Ref 'DBClusterIdentifier'
      DBClusterParameterGroupName: default.aurora-postgresql10
      DBSubnetGroupName: !ImportValue
        Fn::Sub: ${NetworkName}-network-dbsubnet
      DatabaseName: !If
        - UseSnapshot
        - !Ref 'AWS::NoValue'
        - !Ref 'DatabaseName'
      DeletionProtection: 'true'
      Engine: !Ref 'DBEngine'
      EngineVersion: !Ref 'DBEngineVersion'
      KmsKeyId: !Ref 'KmsKeyId'
      MasterUserPassword: !If
        - UseSnapshot
        - !Ref 'AWS::NoValue'
        - !Ref 'Password'
      MasterUsername: !If
        - UseSnapshot
        - !Ref 'AWS::NoValue'
        - !Ref 'Username'
      Port: 5432
      SnapshotIdentifier: !If
        - UseSnapshot
        - !Ref 'ClusterSnapshotIdentifier'
        - !Ref 'AWS::NoValue'
      StorageEncrypted: 'true'
      VpcSecurityGroupIds:
        - !Ref 'DBSecurityGroup'
    Type: AWS::RDS::DBCluster

[adamchainz]

I think that's basically a bug and should be filed separately

Also hint: you don't need quotes around all your Refs, i.e. !Ref DBSecurityGroup is equivalent to !Ref 'DBSecurityGroup'.

[chizou]

E2520's description is "Making sure CloudFormation properties that are exclusive are not defined"

Given that, it's technically correct. MasterUserPassword/MasterUsername and SnapshotIdentifier are mutually exclusive. I can't see how it could be fixed unless cfn-lint was somehow evaluating our conditions to check that we're passing AWS::NoValue correctly.

Thanks for the tip on the quotes. We're using troposphere to generate templates so afaik, I can't disable that.

[kddejong]

@chizou I've been working on a few things to get us closer to proper validation on this. What I really want to get to is analyzing each path in your scenario.

As an example: In your scenario when UseSnapshot is True, MasterUsername is NoValue which would be equivalent it to not existing. We should be able to validate that scenario and determine that there are no conflicting properties when UseSnapshot is True. And vice versa when it is False.

I've taken a few iterations on how to do this efficiently and I think I finally have a good way to do it. #523 is the start to this work. This pull request just checks Relationships between resources and if they exist when Conditions are being used however its built to support the next thing I want to do... which happens to be your scenario (or what I have described above).

So I'm in agreement with @adamchainz that this is a bug based on our current checks. We should be able to test the different scenarios presented to the linter when conditions are being used. #112 is the long running do better with conditions issue I've been trying to track against.

[chizou]

That's great to hear @kddejong. Given that you guys already tracking this issue, should I still file a bug report as @adamchainz suggested?

[dmorel]

@sd65 quick and dirty patch but useful for me (I need to exclude jinja2 syntax in my files):
dmorel@a21c58a

[kddejong]

Resource based exceptions are now available in v0.20.0 using the Resources Metadata.

[tomislacker]

@kddejong Could we get a link posted here of an example or usage for the sake of continuity? I think this issue has been hot enough that it has a fair amount of visibility.

[kddejong]

https://github.com/aws-cloudformation/cfn-python-lint#resource-based-metadata

Is this useful enough? Or any suggestions on improvements?

[tomislacker]

@kddejong Good enough for the girls I go out with, much appreciate!