prevent nil panic

[deads2k]

Fixes #3529, Fixes #3959

I can't reproduce the panic, but this prevents panics in the casts.

[liggitt]

Shouldn't these trigger off non-nil errors, not nil object checks?

[deads2k]

Shouldn't these trigger off non-nil errors, not nil object checks?

I'd rather check for the condition actually causing the failure in case crazy things happen. No matter what the value of the error is, if the obj is nil, it will never be castable.

[liggitt]

Hmm… if it's possible to return nil in a non-error case, that impl is broken. Should sweep for that if we're not confident in it, and short circuit on non-nil err here

[deads2k]

Hmm… if it's possible to return nil in a non-error case, that impl is broken. Should sweep for that if we're not confident in it, and short circuit on non-nil err here

I don't disagree that it would be broken at a lower level. I'm just struggling to come up with a valid reason to avoid checking for the condition that will actually cause an error and handling in a reasonable way.

[liggitt]

I just don't like assuming the value is castable in an error condition.

[deads2k]

I just don't like assuming the value is castable in an error condition.

I don't like assuming I should throw away the returned value in the case of an error. This is supposed to be an object whose only purpose is to pass through to storage in a type-safe way. Assuming an error means a nil return value violates that principle.

[liggitt]

compromise?

obj, err := s.Create(ctx, node)
if role, ok := obj.(*authorizationapi.ClusterRole); ok {
  return role, err
}
if err != nil {
  return nil, err
}
return nil, errors.New("Unexpected type: ...")

[smarterclayton]

You should not cast values if err != nil. The general go convention is "if err != nil, all other return values are suspect unless defined otherwise, and if defined otherwise, shoot the reviewer who allowed it because that's crazy talk".

[deads2k]

I don't like it. Waaa. [merge]

[deads2k]

Oh, just in case you didn't look, I did change it to err != nil, even though I don't like it.

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2884/) (Image: devenv-fedora_2101)

[openshift-bot]

Evaluated for origin merge up to 74a2d68

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3877/)

[openshift-bot]

Evaluated for origin test up to 74a2d68