Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(juju_kubernetes_cloud): get plan from req.Plan instead of req.State #665

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SimoneDutto
Copy link
Contributor

@SimoneDutto SimoneDutto commented Feb 5, 2025

Description

Before we were getting the plan from the req.State, so changes were not propagated in the state.
We now set the plan from req.Plan and set it accordingly in the state.

Fixes:

Type of change

QA steps

Manual QA steps should be done to test this PR.

Create a k8s_config.txt in the project root by doing microk8s config > k8_config.txt

provider juju {}
resource "juju_kubernetes_cloud" "cluster_k8s_cloud" {
  name              = "k8s-stg-is-iam"
  kubernetes_config = file("k8_config.txt")
}

terraform apply
Change something in the config manually: like adding a comment.
terraform apply again.

Now the apply doesn't error out.

Additional notes

Fly by: cleanup of tests. We were getting the MICROK8S config from an environment variable, but this was not propagated to the test's functions correctly in fact there wasn't any {{.Config}} in the terraform plans.

@@ -222,6 +222,7 @@ func (r *kubernetesCloudResource) Update(ctx context.Context, req resource.Updat
return
}

resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This overwrites the ID and credential computed values already saved to state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but unmodified fields are passed from the State to the Plan.
Here the terraform doc is very similar to what we do, and they don't get the ID from the State but from the Plan.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works because of how the schema is defined. You'll have to be careful in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my understanding computed values can't be set by the user, so in the plan are always copied from the state. Making setting the state as the plan always correct for computed values.

internal/provider/resource_kubernetes_cloud_test.go Outdated Show resolved Hide resolved
@SimoneDutto SimoneDutto requested a review from hmlanigan February 6, 2025 16:38
Before we were getting the plan from the req.State, so changes were not propagated in the state.

fix juju#664
@SimoneDutto SimoneDutto force-pushed the fix/kubernetes-cloud-not-updating branch from 7ec45c4 to 06a88a5 Compare February 10, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants