Misc small issues noted while reviewing https://github.com/openshift/hive/pull/2520

Clean up SetErrCondition logging 

cf. comment.

SetErrCondition emits a log message ("setting PrivateLinkFailedClusterDeploymentCondition to true") that is zero or more of:

• inaccurate (if that condition was already true, and the Ready condition is being changed)
• incomplete (if the Ready condition is also being changed)

Evaluate whether we really need logging here at all (and in its cognate, SetReadyCondition) and if so, fix it to emit 0, 1, or 2 messages that accurately reflect what is actually happening.

(NB: This logic/logging was cargo-culted from the original awsprivatelink devtest effort, where it was probably left in by accident in the first place )

ShouldSync should also be checking PrivateLink.Enabled

ShouldSync for AWS and GCP|https://github.com/openshift/hive/blob/2f649fb1c654129fd558cfe309d901cf376cf4eb/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go#L265 should always return false if PrivateLink is not enabled.

CleanupRequired should not be checking PrivateLink.Enabled

...here and here.

In the (albeit contrived) case where Enabled is false but those other fields are set, we would return true and possibly go try to clean up something that doesn't exist.

Update: Jeremiah convinced Eric that this should be left as is.

Idempotence Improvements

Background: As a k8s best practice, a controller is supposed to understand how to proceed from any state such that, if multiple steps are necessary for some overarching operation, it is resilient to interruptions (e.g. controller restarts) at any point. The canonical way to accomplish this is for any path through the reconciler to issue at most one request to update a CR (which, to be clear, may comprise multiple changes) and return, allowing subsequent iterations to pick up the process and move it along to the next stage. In addition to making the controller more resilient, this practice also makes it more easily testable, as a given set of preconditions should yield a set of postconditions with at most a single easily-verified delta.

Here and here are places we're having a hard time unit testing because this path through the reconciler makes multiple changes. Fix it to return after each update. Ensure via existing unit tests that subsequent reconciles correctly resume the operation being performed; and close the test gap.

selectHostedZoneVPC: make return value a pointer

...so we a) aren't copyout-ing a complex var; b) can return nil on error paths, rather than saving a nil-value object for the purpose.

GCP Cleanup: roll up status updates

Currently the GCP link actuator updates status after every cloud API call. Consider instead doing the status updates locally, and only pushing the Status().Update() at the end. (Yes, this may mean that we've cleaned up a given cloud resource but are still listing it in the status. But that's the case anyway, briefly.)

Discuss SetReadyCondition logic

cf. comment

As currently written, SetReadyCondition won't revert the Ready condition from True to False if invoked with completed==ConditionFalse. Why is that? Discuss.