🌱 Refactoring PR for: Remove usage of FailureReason and FailureMessage (baremetal)

[guettli]

Some changes of this PR got extracted, so that the PR gets smaller and easier to review:

🌱 Remove usage of FailureReason and FailureMessage (baremetal) by guettli · Pull Request #1716 · syself/cluster-api-provider-hetzner

• Add/extended comments to some parts.
• move rate-limit handling code up, to return early if rate-limit is active.
• avoid usage of capierrors package.
• test code: Align capi-machine name and infra-machine name. Remediation code depends on that capi feature.
• tee tmp/test-unit-unfiltered.log in test-unis.sh: Write unmodfied output to tmp directory. This is handy if you want to see all logs, without running tests again.
• export func GetAssociatedHost(), so that it can be re-used.
• refactored getAssociatedHost() to getAssociatedHostAndPatchHelper(), so that the code can be re-used.
• add log when hbmm is deleting.
• avoid duplicate implemenation of splitHostKey()
• refactored setOwnerRemediatedConditionNew() to setOwnerRemediatedConditionToFailed()
• removed named return parameters: return res, nil --> return reconcile.Result{}, nil

[janiskemper]

I didn't fully understand the remediation part. Please add some explanation for the refactoring in the PR description

[janiskemper]

can this happen? We handle the not found error... at least it wouldn't make sense

[guettli]

@janiskemper, yes, it can be nil:

func GetAssociatedHost(ctx context.Context, crClient client.Client, hbmm *infrav1.HetznerBareMetalMachine) (*infrav1.HetznerBareMetalHost, error) {
	annotations := hbmm.GetAnnotations()
	// if no annotations exist on machine, no host can be associated
	if annotations == nil {
		return nil, nil
	}

[janiskemper]

okay, but that's weird for that GetAssociatedHost function no? Then we should probably directly handle the not found error and convert it to nil.

Or is there any reason that the function returns in two ways the same thing?

[guettli]

@janiskemper I just refactored the method, so that it could be re-used. It was like this before. For now, I would like to keep it like this. But I could change it, if you think so.

[janiskemper]

I would like to change it. Before, it was not a library function, so it was clear from the inline code. Right now, with having this in a separate package, that looks strange.

[guettli]

I didn't fully understand the remediation part. Please add some explanation for the refactoring in the PR description

can you please point me to a line in the code?

[janiskemper]

@guettli I didn't understand the reason for this part "refactored setOwnerRemediatedConditionNew() to setOwnerRemediatedConditionToFailed()"