Add condition when deprovision fails cloud auth #1109
Conversation
twiest
commented
Sep 1, 2020
•
twiest
commented
- Add condition when deprovision fails cloud auth
- Add unit tests
- Manually test
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
| hivev1.AuthenticationFailureClusterDeprovisionCondition, | ||
| corev1.ConditionTrue, | ||
| authenticationFailedReason, | ||
| err.Error(), |
There was a problem hiding this comment.
These error messages don't contain any request IDs or anything that could change each time through? Just want to be sure we don't hotloop here.
There was a problem hiding this comment.
I'll check to make sure.
| @@ -308,6 +308,9 @@ const ( | |||
|
|
|||
| // InstallLaunchErrorCondition is set when a cluster provision fails to launch an install pod | |||
| InstallLaunchErrorCondition ClusterDeploymentConditionType = "InstallLaunchError" | |||
|
|
|||
| // UninstallPermissionFailure is set when a cluster deprovision fails because the cloud credentials are invalid. | |||
| UninstallPermissionFailure ClusterDeploymentConditionType = "UninstallPermissionFailure" | |||
pkg/controller/clusterdeprovision/clusterdeprovision_controller.go
Outdated
Show resolved
Hide resolved
twiest
force-pushed
the
check_aws_creds_uninstall
branch
3 times, most recently
from
2e1141c
to
3bb84b4
Compare
twiest
force-pushed
the
check_aws_creds_uninstall
branch
2 times, most recently
from
c48f10d
to
e57ac8d
Compare
|
@dgoodwin @joelddiaz I believe I've addressed your comments. PTAL. |
|
/lgtm Needs [wip] removed though. |
twiest
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
twiest
force-pushed
the
check_aws_creds_uninstall
branch
from
e57ac8d
to
24c2b30
Compare
|
@dgoodwin Rebased and removed WIP. No other changes were made. Please re-lgtm. |
| // actuator found, ensure creds work. | ||
| err := actuator.TestCredentials(instance, r.Client, rLog) | ||
| if err != nil { | ||
| rLog.WithError(err).Error("Credential check failed") |
There was a problem hiding this comment.
This should be a warning or an info, not an error. This is a problem with user input not with the functioning of Hive.
| scheme: scheme.Scheme, | ||
| deprovisionsDisabled: test.deprovisionsDisabled, | ||
| } | ||
|
|
||
| // TODO: Mock this | ||
| actuators = []Actuator{&awsActuator{awsClientFn: func(clusterDeprovision *hivev1.ClusterDeprovision, c client.Client, logger log.FieldLogger) (awsclient.Client, error) { |
There was a problem hiding this comment.
This is not safe to do in unit tests. It has side effects which leak out of the test.
There was a problem hiding this comment.
This is still not going to allow for the unit tests to be run in parallel. But I am fine with letting it go as in. We can fix this and the hibernation controller later.
| if err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } |
There was a problem hiding this comment.
Use asserts.
| if err != nil { | |
| t.Errorf("unexpected error: %v", err) | |
| } | |
| assert.NoError(t, err, "unexpected error getting ClusterDeprovision") |
- [x] Add condition when deprovision fails cloud auth - [x] Add unit tests - [x] Manually test
twiest
force-pushed
the
check_aws_creds_uninstall
branch
from
24c2b30
to
b8b7064
Compare
|
@staebler I believe I've addressed your feedback. PTAL. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, staebler, twiest The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
