Closed Itaykal closed 5 months ago
Attention: Patch coverage is 37.68116%
with 43 lines
in your changes missing coverage. Please review.
Project coverage is 40.87%. Comparing base (
5b9c270
) to head (60fb008
). Report is 7 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The PR looks like it would work well. The issue I have with it is the readability, searchability and the extra work to add new resources here. The question is if supporting prefixes and suffixes would be better codewise as that would cover the functionality we are currently lacking. WDYT?
It's quite frustrating to work with... I thought about adding a flag to enable regex searching on specific exceptions that require them and thus simplifying all "non-regex" searches, something along the lines of this:
{
"Namespace": ".*-example",
"ResourceName": "example",
"Regex": true
}
This is also quite unreadable but it grants the efficiency and ease of use of regex while still allowing more readable exceptions when not needed.
Another topic that scares me a bit more then readability is that with some minor human error this could cause false positives. I think this should be discussed before moving on to readability (and ease of use) as I don't see any way to avoid this risk.
It's quite frustrating to work with... I thought about adding a flag to enable regex searching on specific exceptions that require them and thus simplifying all "non-regex" searches, something along the lines of this:
{ "Namespace": ".*-example", "ResourceName": "example", "Regex": true }
This is also quite unreadable but it grants the efficiency and ease of use of regex while still allowing more readable exceptions when not needed.
Another topic that scares me a bit more then readability is that with some minor human error this could cause false positives. I think this should be discussed before moving on to readability (and ease of use) as I don't see any way to avoid this risk.
The damage of a regex too broad would marking unused resources as used which is not great but the damage is less than the reverse.
We obviously want kor to find as many unused resources as possible so we would need to be cautious in this area.
The solution might be to create a separate json for regexes or prefixes and handle it separately. This or the solution you provided with the additional field are likely cleaner and safer
I've implemented the flag solution as it's easier to handle later on, wdyt?
Looks much better. I'd like to have tests in place for this. Also (not necessarily belonging to this PR but we need to have validators for the exception jsons)
What should be tested here? the regex exception's usability or the validity of the expressions themselves?
@Itaykal can you rebase the branch so the exceptions folder will go under validation of the new GitHub action added in #295 and for the kor binary to be ignored as applied in #290?
What should be tested here? the regex exception's usability or the validity of the expressions themselves?
I think we need to test the that it excludes
Rebased and added testing.
@Itaykal many exception duplications were added in this PR (probably following the rebasing), can you create a new PR that removes them?
Bonus - enhance the validation workflow (#298) to remove duplications.
What this PR does / why we need it
Adds regular expression support to
exceptions/*/*.json
files.PR Checklist
Github Issue
Fixes #276
Notes for your reviewers
Not sure this is how this impacts performance. The behavior of the exception filtration in configmaps and serviceaccounts is now changed to be aligned with all other objects, this as well has some impact on performance.