The destination region is set in the ProviderConfig, so we're not setting in the spec of the resource, so I would keep as it is

interesting addition, I would set is as optional to be backward compatible and I would add the optional firewallID and the optional firewallRule because civo API use these. If they're absent (as is situation), we still use the default ones.

I see, the network is actually required in the API, but the Terraform module has this field as optional. This is because when the network is missing, it queries the Civo API to find out the default network ID by doing client.GetDefaultNetwork().

I'll add the same thing here, and it should work to get the network back to optional, and make it consistent with Terraform.

it's very uneasy to generate UUID and pass to the manifest, but it doesn't mean we can sent friendly names like these, because they must be unique in our database. I would left like it is. In the API we can auto generate those, but then it means we cannot update through crossplane if we don't provide a reference at all.

why removing the civocli wrapper object?

civocli looks to be an old package committed to this repo. In the meantime, I see that the new API is implemented in the civogo client. I think this is the right way to go, perhaps the API team can confirm?

if err != nil why removing the error return and return nil error?

Same as above: not sure what meta.GetExternalName would return for a crossplane resource: it should fetch the annotation crossplane.io/external-name but this doesn't correspond to the clusterID as it's requested by the GetKubernetesCluster method (that now, as I see, it's never used).

we don't use region in the spec, it's implicit in the provider configs

I would readd the wrapper function and move the creation of the createCluster request in the originary class. Besides that, if you added optional networkID, firewallID, firewallRule I would pass those to the request OR, in their absence, I would pass the default network ID as it is now, without removing that logic.

this is interesting, effectively then you can use the function you proposed above to directly fetch the cluster by ID :claps

Yes, external-name is Crossplane magic to allow mapping Cloud IDs to Kubernetes IDs. During Create, a controller is supposed to fetch it and write it here.

Civo API uses cloud-generated IDs for resource identification, so this maps to using external-name in Crossplane. It looks like the Civo API always generates IDs, so this should be the main way of doing things. Because you can reference this ID directly to the API in most cases, and you would get the resource back. This is exactly how Upjet providers implement this functionality, so the main AWS/GCP/Azure Crossplane providers all store the Cloud IDs via external-name.

Your comment above about pools IDs is interesting, I need to look more into that. Right now, I don't use the pools api to directly update pools, since there is no pools CRD. So this works for pool updates, just because it uses a cluster-scoped Civo API.

@RealHarshThakur thoughts if we should introduce Region in the crossplane resource's spec? Now we don't have those.

Region is required by the API, so this is why it needs to be passed here. I think the CRD should have it too, it would be good to follow the API as close as possible. And right now, it still picks up the default Region from the ProviderConfig.

Another argument in favour of the region is that if you want to deploy in multiple regions, you can use the same ProviderConfig.