search

open-component-model / ocm

Open Component Model (Software Bill of Delivery Toolset)
https://ocm.software
Apache License 2.0
31 stars 19 forks source link

fix: stop ignoring most of the errors #745

Closed Skarlso closed 2 months ago

Skarlso commented 3 months ago

Description

Fixes https://github.com/open-component-model/ocm-project/issues/40

What type of PR is this? (check all applicable)

Related Tickets & Documents

Screenshots

Added tests?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

Checklist:

github-actions[bot] commented 3 months ago

Mend Scan Summary: :x:

Repository: open-component-model/ocm

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 1
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 4
HIGH RISK LICENSES 9
RESTRICTIED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report Mend UI

ocmbot[bot] commented 3 months ago

Integration Tests for 600f585d767aeaef3f5a6cfdd25b1db337d29b3e run with result: Success ✅!

ocmbot[bot] commented 3 months ago

Integration Tests for 600f585d767aeaef3f5a6cfdd25b1db337d29b3e run with result: Success ✅!

Skarlso commented 3 months ago

I'm quite new to go, so please bear with me.

For those cases, where the method signature doesn't allow to return an error, shouldn't we then at least log them?

We can attempt to but it's not always possible because the logger is just present or it would require changing the interface to pass it in which is simply too disruptive.

ocmbot[bot] commented 3 months ago

Integration Tests for 4b9a12f8017bf51712c03253d3c4ac2071ce522e run with result: Success ✅!

ocmbot[bot] commented 3 months ago

Integration Tests for 4b9a12f8017bf51712c03253d3c4ac2071ce522e run with result: Success ✅!

ocmbot[bot] commented 2 months ago

Integration Tests for ea79db625a33260006585329f1e2cc10ae33e6e0 run with result: Success ✅!

ocmbot[bot] commented 2 months ago

Integration Tests for e05a5a083e96085114b24d03925f614683d9e8cc run with result: Success ✅!

Skarlso commented 2 months ago

@hilmarf can we get on with this, please? I would rather have errors handled than this being stuck in review, forever open over whether or not we log errors.

We can open a separate issue for handling logging but this is already a massive improvement on errors being completely ignored otherwise and not surfaced at all, in most of the cases.

With staff being bogged down and in low numbers, I would rather iterate fast and solve issues as they arise than be in review forever.

hilmarf commented 2 months ago

@hilmarf can we get on with this, please? I would rather have errors handled than this being stuck in review, forever open over whether or not we log errors.

We can open a separate issue for handling logging but this is already a massive improvement on errors being completely ignored otherwise and not surfaced at all, in most of the cases.

With staff being bogged down and in low numbers, I would rather iterate fast and solve issues as they arise than be in review forever.

totally fine with me, but you had a nice pattern for identifying the problematic code areas... now with handling them, but don't logging them, still my fear is, that we silently drop errors and won't even be able to find the problematic coding later on, because there is no search pattern anymore available...

so please: either try to log the errors (and yes, I don't care where those logs might appear) OR at least add a FIXME / TODO comment, where the errors are not populated up.

Skarlso commented 2 months ago

so please: either try to log the errors (and yes, I don't care where those logs might appear) OR at least add a FIXME / TODO comment, where the errors are not populated up.

👍

ocmbot[bot] commented 2 months ago

Integration Tests for 8a4d9192ede06a32c789d813e6100942c70144ea run with result: Success ✅!

Skarlso commented 2 months ago

@hilmarf Done. :)