Skip to content

Rule suppression does not work for non builtin rules #584

Closed

Description

@kapilmb

The current implementation supports rule suppression only for builtin rules.

Activity

  1. added this to the milestone on Jul 19, 2016
  2. charlieschmidt commented on Jul 20, 2016

    @charlieschmidt
    Contributor

    Kapil Borle (@kapilmb) if it's low on the backlog and if you can point me to the best "testing" method - ie how does one build from source and the best/easiestly test those changes, I'm happy to take a stab at it here. I imagine probably using scriptanalyzer as the C# library for VS debugging is probably easiest, so write a little stub application that invokes the analyzer with the params I'd need on a module with my custom rules & functions? Or is there something more elegant for testing/debugging?

  3. charlieschmidt commented on Jul 21, 2016

    @charlieschmidt
    Contributor

    Kapil Borle (@kapilmb) stab at it here: https://github.com/charlieschmidt/PSScriptAnalyzer

    Problems:

    1. ends up doing the external rules 1 by 1, instead of the runspacepool that was there already - i suspect theres a way around that, but the GetExternalRecord() method is a bit complicated at first glance, so wasn't sure the best way to do it.
    2. the powershell methods that are the rules have to match the diagnosticrecord.rulename exactly - cause the initial ExternalRule.GetName() just has the function name, but the supressionrule list and diagnosticrecord list might have something else and it won't match any longer. which isn't awesome necessarily, but also isn't that horrible of a requirement. The alternative would probably be to modify the ExternalRule after you get the first diagnostic record for it so it has the "real" name maybe, or to modify the SupressRecord method so it does something fancier when matching external rules and supressed records - but then that ends up being a lame exceptional case.

    Initial thoughts on either of those problems? Happy to keep working at it. I suspect (1) is probably just a matter of rejiggering the responses from GetExternalRecord so it can still run the rules in parallel and return the appropriate lists. (2) - dunno.

  4. kapilmb commented on Jul 21, 2016

    @kapilmb
    Author

    Charlie Schmidt (@charlieschmidt) Thanks for the effort.

    1. I have left some comments on your commit.
    2. Yes, you are right - There is a potential for inconsistency which can render rule suppression useless. But, your proposal of getting the "real" rule name after examining diagnostic record appears to be promising. Basically, once we have the diagnostic records we can examine them for unique rule names and run suppression using those rule names.
  5. modified the milestones: 1608, on Aug 3, 2016
  6. kapilmb commented on Aug 3, 2016

    @kapilmb
    Author

    Charlie Schmidt (@charlieschmidt) Merged your contribution in PR #588. Thanks again!

    I am leaving this open, as #588 fixes suppression for only AST based external rules. So that leaves the token based external rules not subject to suppression. Will add that through another PR then close this issue.

  7. kapilmb commented on Sep 6, 2016

    @kapilmb
    Author

    Closing this as the fix in #588 fixes token based external rule suppression too!

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

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions