Add e2e test to cover region in providerID clusterv1 machine object

[MatthieuFin]

What this PR does / why we need it:
Add E2E test to cover region in providerID clusterv1 machine object introduced in issue #2183 and PR #2193

Special notes for your reviewer:

I have write 2 test cases:

1. One cluster object with identityRef.Region field specified in OpenStackCluster providerID-with-region-default and no identityRef specified in OpenStackMachineTemplate
2. One cluster object without identityRef.Region field specified in OpenstackCluster providerID-with-region-override and identityRef.Region specified in OpenStackMachineTemplate.

I uncounter an error on test case number 1, region name missing from clusterv1.Machine providerID string. That why I add a 2nd commit to fix that usecase and keep behavior of identityRef herited from Cluster Object.

It seems to do the job but it is "semantically" not really fair because I mutate object openStackMachine to append Cluster idenityRef if openStackMachine one's is nil, but this object goal of life is to mirror reality so that's technically a hack should I append a parameter to reconcileMachineState function to pass cluster object or cluster's identityRef object ?

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jichenjc for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @MatthieuFin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	e1edef9
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6723a9707a7a2e00089b7463
😎 Deploy Preview	https://deploy-preview-2210--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mdbooth]

/ok-to-test

[MatthieuFin]

Outch, I thinked that CAPO_REGION env variable will be set by https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/templates/env.rc#L89 but it seems not, personally I run tests by setting my own CAPO_REGION variable with my region name.
How could I retrieve CI region name to feed CAPO_REGION env variable ?

[MatthieuFin]

/unhold

[EmilienM]

I'm curious why you need to do that here. We already do it here:
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/2210/files#diff-e0a0984c0e26655878e2f94623fbf8cddb4d8a89e3443c1296dfaadc8802838aR578-R583

I'll like to avoid dupplications.

[MatthieuFin]

function getOrCreateMachineServer build an openStackServer object, that's not the purpose of this context so I didn't think to use it like that.
Do you think that this way is a better approach ?

[EmilienM]

No it's not a better approach I think.
However moving the code to a separated function and use it in both places seems a good idea. We share the logic in case it has to change one day.

Thanks for trying that.

[MatthieuFin]

Obviously it seems to make sens, I'll take a look on this way.

[MatthieuFin]

IdentityRef lookup logic was also implmented here, so I tried to factorise code to use same implementation at anyplaces with this last commit.

[EmilienM]

Really good start, thanks for writing these tests! A few minor comments inline.

[EmilienM]

I'm not trying to be picky but I want to ensure we create code that will be maintainable and debuggable. I don't see error management here. What if the identifyRef wasn't found? Shouldn't we return an error?
Or from the callers, if identityRef is nil, we should return an error?

I think either way we need to stop and send a clean error if we can't find an identityRef. Especially because we use a pointer in one of the cases and I like to see pointer checks before using them.

[MatthieuFin]

No problem, I ask myself about that too.
I took the choose of that the responsibility of caller since objects ...infrav1.IdentityRefProvider could have nil pointer to IdentityRef field, I don't wanna return an error because the value of objects could be nil as a normal behavior.

In my 2 callers functions last parameters are openStackCluster type of infrav1.OpenStackCluster which is not a pointer https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/api/v1beta1/openstackcluster_types.go#L194 so I'm sure that identityRef variable here and here and correctly defined.

But I'm agree maybe tomorrow it could be different, so I could validate function return from caller for more maintainability.