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

Fix SARIF-formatter severity levels #3824

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Fix SARIF-formatter severity levels #3824

merged 7 commits into from
Oct 10, 2023

Conversation

4ch1m
Copy link
Contributor

@4ch1m 4ch1m commented Oct 8, 2023

The SARIF formatter output contains information about "severity levels" in two different contexts (which should be handled differently).

  • general "rule-based" severity levels
  • "error-/match-based" severity levels within the result set

@4ch1m 4ch1m requested a review from a team as a code owner October 8, 2023 18:01
@4ch1m 4ch1m temporarily deployed to ack October 8, 2023 18:01 — with GitHub Actions Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to ack October 8, 2023 18:02 Inactive
@4ch1m 4ch1m temporarily deployed to ack October 8, 2023 18:18 — with GitHub Actions Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to ack October 8, 2023 18:19 Inactive
@4ch1m 4ch1m temporarily deployed to ack October 8, 2023 18:41 — with GitHub Actions Inactive
@ssbarnea
Copy link
Member

ssbarnea commented Oct 9, 2023

@4ch1m While I am aware that we dump severity in different places, I do not think that is an issue by itself, mainly because once would be the severity of a rule in general and the other one would specific to a particular occurence.

Still, the title of the PR does not mention or refer the exact bug it is addressing. I am asking this as I want to fully understand what was wrong before. Thanks

@4ch1m
Copy link
Contributor Author

4ch1m commented Oct 9, 2023

Sure... I'll try to explain.

The current implementation basically is using this function to obtain the severity (for both the general rule and the specific occurrence:

@functools.cached_property
def level(self) -> str:
"""Return the level of the rule: error, warning or notice."""
if not self.ignored and {self.tag, self.rule.id, *self.rule.tags}.isdisjoint(
options.warn_list,
):
return "error"
return "warning"

NOTE: The result is "influenced" by options.warn_list.

External tools (IDE plugins, etc.) will use the SARIF output (and the given severity levels) to highlight/annotate Ansible code; which is problematic in many ways.

Example:
I just recently realized that all of my "name[casing]" issues were marked as errors (whereas their severity is actually only MEDIUM).

External tools should be able to give hints/advice about rules in general (with the rules original/general severity) and the actual error/occurrence.

That's why this should be fixed as proposed in this PR. 😄

@4ch1m 4ch1m temporarily deployed to ack October 9, 2023 14:34 — with GitHub Actions Inactive
@ssbarnea ssbarnea added the bug label Oct 10, 2023
@ssbarnea ssbarnea temporarily deployed to ack October 10, 2023 18:03 — with GitHub Actions Inactive
@ssbarnea ssbarnea temporarily deployed to ack October 10, 2023 18:09 — with GitHub Actions Inactive
@ssbarnea ssbarnea changed the title fix SARIF-formatter severity levels Fix SARIF-formatter severity levels Oct 10, 2023
@ssbarnea ssbarnea merged commit 7083eec into ansible:main Oct 10, 2023
22 of 23 checks passed
nrdufour added a commit to nrdufour/home-ops that referenced this pull request Oct 20, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ansible-lint](https://github.com/ansible/ansible-lint) ([changelog](https://github.com/ansible/ansible-lint/releases)) | minor | `==6.20.3` -> `==6.21.1` |

---

### Release Notes

<details>
<summary>ansible/ansible-lint (ansible-lint)</summary>

### [`v6.21.1`](https://github.com/ansible/ansible-lint/releases/tag/v6.21.1)

[Compare Source](ansible/ansible-lint@v6.21.0...v6.21.1)

#### Bugfixes

-   Avoid exception caused by accidental unloading of core rules ([#&#8203;3857](ansible/ansible-lint#3857)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Document pre-commit access to ansible community bundle ([#&#8203;3856](ansible/ansible-lint#3856)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Fix bug with auto-fix ending too soon ([#&#8203;3855](ansible/ansible-lint#3855)) [@&#8203;ssbarnea](https://github.com/ssbarnea)

### [`v6.21.0`](https://github.com/ansible/ansible-lint/releases/tag/v6.21.0)

[Compare Source](ansible/ansible-lint@v6.20.3...v6.21.0)

#### Minor Changes

-   Allow linting plugin EXAMPLES as playbooks ([#&#8203;3309](ansible/ansible-lint#3309)) [@&#8203;Qalthos](https://github.com/Qalthos)

#### Bugfixes

-   Add support for Rocky ([#&#8203;3843](ansible/ansible-lint#3843)) [@&#8203;facorazza](https://github.com/facorazza)
-   Update supported Ubuntu versions in `meta.json` ([#&#8203;3845](ansible/ansible-lint#3845)) [@&#8203;mcdonnnj](https://github.com/mcdonnnj)
-   Avoid false positives for handler in roles handlers directory ([#&#8203;3838](ansible/ansible-lint#3838)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Hide stacktrace when loading invalid yaml ([#&#8203;3844](ansible/ansible-lint#3844)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Add some platforms to `meta.json` ([#&#8203;3841](ansible/ansible-lint#3841)) [@&#8203;mcdonnnj](https://github.com/mcdonnnj)
-   Temporary avoid auto-fixing YAML files not owned by ansible ([#&#8203;3837](ansible/ansible-lint#3837)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Add environment variable for skipping schema update ([#&#8203;3835](ansible/ansible-lint#3835)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Avoid creating temporary YAML files inside source tree ([#&#8203;3819](ansible/ansible-lint#3819)) [@&#8203;Qalthos](https://github.com/Qalthos)
-   Document environment variables ([#&#8203;3833](ansible/ansible-lint#3833)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Update schemas ([#&#8203;3832](ansible/ansible-lint#3832)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Support complex requirements in argument_specs.yml ([#&#8203;3823](ansible/ansible-lint#3823)) [@&#8203;tapetersen](https://github.com/tapetersen)
-   Fix SARIF-formatter severity levels ([#&#8203;3824](ansible/ansible-lint#3824)) [@&#8203;4ch1m](https://github.com/4ch1m)
-   Add play level autofix for key-order rule ([#&#8203;3815](ansible/ansible-lint#3815)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Add support for python 3.12 ([#&#8203;3813](ansible/ansible-lint#3813)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Update SPDX license list ([#&#8203;3814](ansible/ansible-lint#3814)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Use checkout action in install docs ([#&#8203;3810](ansible/ansible-lint#3810)) [@&#8203;gma](https://github.com/gma)
-   Fix actions-tagger arguments ([#&#8203;3808](ansible/ansible-lint#3808)) [@&#8203;ssbarnea](https://github.com/ssbarnea)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNy4wIiwidXBkYXRlZEluVmVyIjoiMzcuMjcuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Reviewed-on: https://git.home/nrdufour/home-ops/pulls/161
Co-authored-by: Renovate <renovate@ptinem.io>
Co-committed-by: Renovate <renovate@ptinem.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants