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

Configurable violation ignore list file #1584

Closed
ssbarnea opened this issue May 24, 2021 · 6 comments · Fixed by #3004
Closed

Configurable violation ignore list file #1584

ssbarnea opened this issue May 24, 2021 · 6 comments · Fixed by #3004
Assignees
Labels
feature feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it.

Comments

@ssbarnea
Copy link
Member

Summary

I would like to implement a configurable option for silencing specific errors similar to the one used by flake8, where you can acknowledge and ignore specific errors inside specific files, even mentioning the line numbers of you want.

If we do this we may even avoid the need to add YAML comments with noqa, which can be really spammy and only record exceptions in ignore.txt files, very similar with what ansible-test does now.

Details

While initially this can be implemented as a feature, it does open the door of removing ruyaml loading completely and rely on pyyaml alone, simplifying the corebase considerably.

Still, decision about if we should remove noqa comments or not should require a wider community consultation.

Example from flake8 config:

per-file-ignores =
  docs/conf.py: D
  src/molecule/test/*: D100,D103,D104

As one can see this allows to disable rules on files and patterns, being very easy to maintain.

Please use thumbs up down or comments to provide feedback on this proposal.

@ssbarnea ssbarnea added feature feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. labels May 24, 2021
@webknjaz webknjaz added the new Triage required label Sep 7, 2021
@ssbarnea ssbarnea removed the new Triage required label Jul 6, 2022
@ssbarnea ssbarnea self-assigned this Aug 16, 2022
@ssbarnea
Copy link
Member Author

ssbarnea commented Aug 16, 2022

I was looking at this proposal and trying to find a good way to represent these ignores inside YAML as we don't want to parse the strings to implement it like pylint. I came with 3 ideas:

# new feature file rule skips (V1)
skips:
  git-latest:
    - "*.*"

# V2
skips:
  "*.*": git-latest

# V3 not the exclude_paths already exists at root level and has the same format, this one has
# the advantage of allowing use to add other rule specific configuration in the future.
rules:
  git-latest:
    exclude_paths:
      - "*.*"

WDYT?

@cidrblock
Copy link
Contributor

cidrblock commented Aug 16, 2022

seems sane to me, does this leave the door open for subrule disable support?

rules:
  name:
    exclude_paths:
      - "*.*"
    subrules:
      - block: false
      - import_play: true #default
      - tasks: true
      - imperative: false

@thaumos
Copy link

thaumos commented Aug 16, 2022

I like v3, that seems the cleanest and easily legible as to what is being accomplished.

@nogweii
Copy link

nogweii commented Sep 5, 2022

I've run into needing this a couple of times in my own infrastructure. I've been able to work around it by extracting the relevant bits of YAML into a separate file and excluding it using exclude_paths. However, I feel like that disables too much.

The V3 proposal is very nice and would work for my use case, being able to disable only certain violations on a file rather than all of them.

@ghost
Copy link

ghost commented Oct 11, 2022

Rather than deprecate noqa and dump the concept entirely, perhaps it should be considered for a rework, similar to how cfn-lint works? An extra property on the ansible task or such to indicate the rule or rules to disable would remove the need for the parsing that seems to be the subject of concern. In the event that Ansible reacts poorly to the extra property, then that's worth a change request in and of itself to Ansible to better enable metadata stapling to tasks/handlers/etc. to support a variety of tools - the only thing that would need to be ensured is that one specific property is outright ignored, it's up to the other tools to decide on their own sub-namespaces and interoperation after that point.

I say this as any attempt at tying ignores to line numbers is going to be extremely brittle and drive users of the tool into wholesale disabling of the rules themselves rather than pinpoint where the rule is undesired, leading to a net loss in quality assurance for the resulting product and reduce the value that ansible-lint provides to users.

ssbarnea added a commit that referenced this issue Feb 9, 2023
ssbarnea added a commit that referenced this issue Feb 9, 2023
ssbarnea added a commit that referenced this issue Feb 10, 2023
@ssbarnea ssbarnea changed the title Configurable violation ignore list and potential deprecation of in-line noqa comments Configurable violation ignore list file Feb 10, 2023
@ssbarnea
Copy link
Member Author

ssbarnea commented Feb 10, 2023

For the moment we will not deprecate any inline noqa. We have a proposed implementation that use the same file format as ansible-test ignore files at #3004

We decided to go for a simple text file because that would make it much easier to update by other tools, which could just append entries to the file, something that would not be possible if we would use the YAML config file.

ssbarnea added a commit that referenced this issue Feb 10, 2023
ssbarnea added a commit that referenced this issue Feb 10, 2023
ssbarnea added a commit that referenced this issue Feb 11, 2023
ssbarnea added a commit that referenced this issue Feb 11, 2023
ssbarnea added a commit that referenced this issue Feb 11, 2023
ssbarnea added a commit that referenced this issue Feb 11, 2023
ssbarnea added a commit that referenced this issue Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants