Add error messages to test framework when changing user

[tnozicka]

Helps uncover #17805 and those error mesages should have been there anyways.

[tnozicka]

/cc @deads2k

[tnozicka]

flake #17829
/retest

[tnozicka]

flake #17829 again
/retest

[deads2k]

@tnozicka This ends up hiding the original error types. That causes some calling code (sometimes multiple levels up) to fail. See comment on error handling for RequestToken call sites.

[mfojtik]

@tnozicka agree with David, can we throw there some V(5) glog.Infos instead?

[tnozicka]

I'll check it but I don't think that any non direct caller should check those error types for exactly this reason, or am I wrong?

[tnozicka]

/retest

[tnozicka]

@deads2k would you mind sharing where that is used? I've found possibly only

origin/test/integration/oauth_basicauth_test.go

Lines 357 to 369 in 1eed81f

	accessToken, err := tokencmd.RequestToken(&anonConfig, nil, expectedLogin, expectedPassword)
	// Expected error
	if !tc.ExpectSuccess {
	if err == nil {
	t.Errorf("%s: Expected error, got token=%v", k, accessToken)
	} else if statusErr, ok := err.(*apierrs.StatusError); !ok {
	t.Errorf("%s: expected status error, got %#v", k, err)
	} else if statusErr.ErrStatus.Code != tc.ExpectErrStatus {
	t.Errorf("%s: expected error status %d, got %#v", k, tc.ExpectErrStatus, statusErr)
	}
	continue
	}

and integration passed.

[deads2k]

@deads2k would you mind sharing where that is used? I've found possibly only

This is usually an exercise for the reader. First one is free, but the second counts as a favor. :)

1. https://github.com/openshift/origin/blob/master/pkg/cmd/util/tokencmd/request_token.go#L151
2. https://github.com/openshift/origin/blob/master/pkg/cmd/util/tokencmd/request_token.go#L73
3. https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/loginoptions.go#L227
4. https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/loginoptions.go#L75
5. https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/login.go#L187
6. https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/login.go#L70
7. checked here https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/login.go#L72

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tnozicka
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• pkg/cmd/OWNERS
• pkg/oc/OWNERS
• test/integration/OWNERS [tnozicka]
• test/util/OWNERS [tnozicka]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[tnozicka]

thanks @deads2k, I didn't want to abuse your time but I though you already knew the place (based on your previous comment) and the path to it from the low level seemed like backtracing all the calls 7 level up with exponential growth.

I went with github.com/pkg/errors so the underlying type can be unwrapped.

[tnozicka]

flakes: #17605 #17901 #17900
/retest

[tnozicka]

/retest

[soltysh]

This lgtm, but I'll defer to David for final signoff.

[deads2k]

The problem with doing it this way is that it's no longer the standard way of dealing with a golang error and our project standard isn't to attempt unwrapping. Having done this in java, you either have to convert the entire code base or you don't do it at all. Mix and match fails unexpectedly in frustrating spots.

[tnozicka]

I would look at it from different point. The way it was done was non-standard - checking error type is "allowed" only for the direct caller and this rouge code did it several layers up. The standard Go way is to wrap an error into a string (erasing the type), adding context. This wrapping makes the error look the same way as if it was wrapped normally into a string for any caller not aware of the wrapping - fulfilling the standard we have. In addition it allows unwrapping to fix the places where we were breaking the standard way by checking error type several layers up.

Also the package is already vendored.

[openshift-merge-robot]

@tnozicka PR needs rebase

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close