🐛 Resolve Deppy error check todo #535
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
m1kola
force-pushed
the
resolve-deppy-error-check-todo
branch
from
262f36f
to
088c9b7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
- Coverage 84.72% 84.53% -0.19%
==========================================
Files 23 23
Lines 910 912 +2
==========================================
Hits 771 771
- Misses 95 96 +1
- Partials 44 45 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| func prettyUnsatMessage(err error) string { | ||
| unsat := deppy.NotSatisfiable{} | ||
| if !errors.As(err, &unsat) { | ||
| panic("we must always receive deppy.NotSatisfiable here") |
There was a problem hiding this comment.
I'm always hesitant about introducing panics in code, could you explain the benefit?
There was a problem hiding this comment.
prettyUnsatMessage is a workaround operator-framework/deppy#142 (there is a comment about it above, not in the diff). It will go away and we will be doing something like this in controller code eventually:
setResolvedStatusConditionFailed(&op.Status.Conditions, err.Error(), op.GetGeneration())I was thinking failing laudly given that this is a temporary code and this condition should never happen unless we introduce a regression. But now I'm thinking about it again - yes, probably there is not much benefit in this and we can do just something like this:
unsat := deppy.NotSatisfiable{}
if !errors.As(err, &unsat) {
return err.Error()
}I'll update.
We already have a version which contains the fix and can revert to a simpler check. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
m1kola
force-pushed
the
resolve-deppy-error-check-todo
branch
from
088c9b7
to
7fb8b6d
Compare
3373785
Description
We already have a version which contains the fix and can revert to a simpler check.
Reviewer Checklist