civo / terraform-provider-civo

Terraform Civo provider
https://www.civo.com
Mozilla Public License 2.0
69 stars 53 forks source link

[BUG] Changing the applications field in the civo_kubernetes_cluster resource does nothing #258

Closed fernando-villalba closed 1 month ago

fernando-villalba commented 1 month ago

Description

Issue

Changing the applications field after creation does absolutely nothing.

provider "civo" {
  region = "LON1"
}

data "civo_size" "xsmall" {
    filter {
        key = "type"
        values = ["kubernetes"]
    }

    sort {
        key = "ram"
        direction = "asc"
    }
}

resource "civo_kubernetes_cluster" "my-cluster" {
    name = "my-cluster"
    applications = "mariadb:5GB" # comment or change this
    firewall_id = civo_firewall.my-firewall.id
    pools {
        label = "front-end" // Optional
        size = element(data.civo_size.xsmall.sizes, 0).name
        node_count = 2
    }
}

Acceptance Criteria

Sadly, we don't have a better way to deal with these apps at the moment, some support removal in the CLI but it's not very consistent at all. Perhaps in the future that will change.

Screenshots

No response

Additional information

No response

Praveen005 commented 1 month ago

Can I work on this @fernando-villalba ?

uzaxirr commented 1 month ago

Can you highlight your approach please @Praveen005

Praveen005 commented 1 month ago

Hi @uzaxirr,

Hope you are doing well.

Regarding this issue, I believe we need to return an error from the if() block here, https://github.com/civo/terraform-provider-civo/blob/c7c3c372902272f84ac0c59627efc834298d1dc1/civo/kubernetes/resource_kubernetes_cluster.go#L410-L413

However, the problem is that this if() block is unreachable if the pools attribute hasn't changed. https://github.com/civo/terraform-provider-civo/blob/c7c3c372902272f84ac0c59627efc834298d1dc1/civo/kubernetes/resource_kubernetes_cluster.go#L371-L373

So, if we just move the if() block above it, this issue gets resolved as evident below.

Screenshot 2024-07-17 212246

However, this feels like a patch. so, I am trying to figure out, why is the return statement there in the first place. https://github.com/civo/terraform-provider-civo/blob/c7c3c372902272f84ac0c59627efc834298d1dc1/civo/kubernetes/resource_kubernetes_cluster.go#L372

Once I figure out, If you allow, I will raise a PR for review.

Regards.

Praveen005 commented 1 month ago

Hi @uzaxirr,

I am not sure what purpose the if() block below from resourceKubernetesClusterUpdate() serves. Is it there only to allow updates to the Kubernetes cluster when the pools attribute has undergone some update/change?

https://github.com/civo/terraform-provider-civo/blob/c7c3c372902272f84ac0c59627efc834298d1dc1/civo/kubernetes/resource_kubernetes_cluster.go#L371-L373

If that is the case, moving the HasChange check for applications above this code block would be the appropriate solution. If not, this piece of code could potentially be removed:

Could you kindly check once?

Thank you!

uzaxirr commented 1 month ago

Solved via https://github.com/civo/terraform-provider-civo/pull/267