treat fatal errors as actually fatal #12842
Conversation
bparees
force-pushed
the
fatal_retries
branch
2 times, most recently
from
acdc780 to
8664b9d
Compare
|
@jim-minter ptal. among other things, we were not recognizing fatal errors as being fatal because they were pointers and our check only looked for non-pointer types. Also am now treating Forbidden as a fatal error. (we get a fatal error when the namespace is being deleted and we try to instantiate into it). |
|
[test] |
|
[testextended][extended:core(builds)] |
| _, isFatal := err.(ConfigControllerFatalError) | ||
| return isFatal | ||
| switch err.(type) { | ||
| case ConfigControllerFatalError, *ConfigControllerFatalError: |
There was a problem hiding this comment.
I think this is bad news - is it possible to fix the one place where we return &ConfigControllerFatalError (as opposed to ConfigControllerFatalError) instead?
There was a problem hiding this comment.
I considered it, but nothing is going to stop someone else from going it in the future, so i'd want to check both regardless.
| _, isFatal := err.(FatalError) | ||
| return isFatal | ||
| switch err.(type) { | ||
| case FatalError, *FatalError: |
There was a problem hiding this comment.
Is there anywhere in the codebase that returns &FatalError?
There was a problem hiding this comment.
not today, but again, who knows what someone does tomorrow.
pkg/build/generator/generator.go
Outdated
| _, isFatal := err.(GeneratorFatalError) | ||
| return isFatal | ||
| switch err.(type) { | ||
| case GeneratorFatalError, *GeneratorFatalError: |
There was a problem hiding this comment.
I think there's only one &GeneratorFatalError too
|
@jim-minter I agree that in theory we can fix the code so it all works with non-pointer types, but I wanted to be defensive against what someone does in the future... do you see an issue w/ checking for both types, even if we aren't using both today? |
|
OK, I stand by my instinct but realise my reasoning is wrong :-) I think the actual issue is that we're not using a pointer receiver on If these lines are re-written to |
|
this is one of the reasons i loathe golang's "it's sometimes a pointer" approach to pointers. |
bparees
force-pushed
the
fatal_retries
branch
from
8664b9d to
361b63f
Compare
|
@jim-minter updated, ptal |
|
@bparees lgtm |
|
Evaluated for origin testextended up to 361b63f |
|
Evaluated for origin test up to 361b63f |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13665/) (Base Commit: c1b17a1) |
|
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1074/) (Base Commit: c1b17a1) (Extended Tests: core(builds)) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13665/) (Image: devenv-rhel7_5877) |
|
Evaluated for origin merge up to 361b63f |
fixes #6388