-
Notifications
You must be signed in to change notification settings - Fork 971
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 kubernetes_job updates #769
Conversation
@@ -384,6 +384,7 @@ func containerFields(isUpdatable, isInitContainer bool) map[string]*schema.Schem | |||
}, | |||
"image": { | |||
Type: schema.TypeString, | |||
ForceNew: !isUpdatable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only added ForceNew: !isUpdatable
here as that's what made me notice the issue, but it should probably be generalized to other fields.
@@ -36,14 +36,12 @@ func jobSpecFields() map[string]*schema.Schema { | |||
"active_deadline_seconds": { | |||
Type: schema.TypeInt, | |||
Optional: true, | |||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to force recreation of the resource so the previous acceptance passed anyway: https:/terraform-providers/terraform-provider-kubernetes/pull/769/files#diff-de5bc44a1e838d53cd603bc71a631b48R74
@@ -82,7 +82,13 @@ func resourceKubernetesJobUpdate(d *schema.ResourceData, meta interface{}) error | |||
} | |||
|
|||
ops := patchMetadata("metadata.0.", "/metadata/", d) | |||
|
|||
if d.HasChange("spec") { | |||
specOps, err := patchJobSpec("/spec", "spec.0.", d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patchJobSpec()
was not invoked anywhere before.
resource.TestCheckResourceAttr("kubernetes_job.test", "spec.0.template.0.spec.0.container.0.name", "hello"), | ||
resource.TestCheckResourceAttr("kubernetes_job.test", "spec.0.template.0.spec.0.container.0.image", "alpine"), | ||
), | ||
}, | ||
{ | ||
Config: testAccKubernetesJobConfig_recreated_image(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexsomesan do you know of a way to ensure a resource is updated or recreated during acceptance test steps checks?
In the context of kubernetes, this could probably be achieved comparing resource uid
between steps, but it is not trivial, and it could most certainly be achieved more generically at the terraform TestStep
level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't any canonical way. For Kubernetes specifically I would look at metadata.generation
or metadata.creation_timestamp
and track those between the different test steps. I'm not entirely sure how generation
behaves between updates, but the creation timestamp should not change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try this. In the mean time, I found https:/hashicorp/terraform-plugin-sdk/issues/77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've submitted hashicorp/terraform-plugin-sdk#329, based on hashicorp/terraform-plugin-sdk#222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdecat you were right about looking at the UID, btw. I've been adding a bunch of checks to ensure resources aren't deleted and re-created. I built off of someone else's work who implemented the ForceNew checks in the provider tests. So that's how the UID comparison is done.
Acceptance test results with the new
|
ecb65a3
to
b8defeb
Compare
…reated/updated/recreate between acceptance test steps
b8defeb
to
7f9e6d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this PR is still useful, so I'm going to remove all references to ExpectedDiffChanges
that I was trying to introduce in hashicorp/terraform-plugin-sdk#329 PR which was declined because of the Terraform SDK v2 transition.
@@ -67,7 +67,8 @@ func TestAccKubernetesJob_basic(t *testing.T) { | |||
CheckDestroy: testAccCheckKubernetesJobDestroy, | |||
Steps: []resource.TestStep{ | |||
{ | |||
Config: testAccKubernetesJobConfig_basic(name), | |||
Config: testAccKubernetesJobConfig_basic(name), | |||
ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffCreate}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hashicorp/terraform-plugin-sdk#329 PR that was introducing ExpectedDiffChanges
was declined because of the Terraform SDK v2 transition.
@@ -86,7 +87,29 @@ func TestAccKubernetesJob_basic(t *testing.T) { | |||
), | |||
}, | |||
{ | |||
Config: testAccKubernetesJobConfig_modified(name), | |||
Config: testAccKubernetesJobConfig_modified(name), | |||
ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffUpdate}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hashicorp/terraform-plugin-sdk#329 PR that was introducing ExpectedDiffChanges
was declined because of the Terraform SDK v2 transition.
}, | ||
{ | ||
Config: testAccKubernetesJobConfig_recreated_selector(name), | ||
ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffDestroyCreate}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hashicorp/terraform-plugin-sdk#329 PR that was introducing ExpectedDiffChanges
was declined because of the Terraform SDK v2 transition.
resource.TestCheckResourceAttr("kubernetes_job.test", "spec.0.template.0.spec.0.container.0.name", "hello"), | ||
resource.TestCheckResourceAttr("kubernetes_job.test", "spec.0.template.0.spec.0.container.0.image", "alpine"), | ||
resource.TestCheckNoResourceAttr("kubernetes_job.test", "wait_for_completion"), | ||
), | ||
}, | ||
{ | ||
Config: testAccKubernetesJobConfig_recreated_image(name), | ||
ExpectedDiffChanges: map[string]terraform.DiffChangeType{"kubernetes_job.test": terraform.DiffDestroyCreate}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hashicorp/terraform-plugin-sdk#329 PR that was introducing ExpectedDiffChanges
was declined because of the Terraform SDK v2 transition.
Some terrafmt errors are not picked up by the CI:
|
Thanks for this PR! I just noticed it. I happen to have a bunch of related work in progress. I think I can fit your changes into my PR, since it's related to PodSpec. I'll make sure you get credit for the work too (co-author in github). |
I pulled your changes into this PR #1074, so I'll go ahead and close this old PR. Thanks for these contributions! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
This is marked as WIP because it needs:
Add ExpectedDiffChanges to assert expected diff change type in acceptance test steps terraform-plugin-sdk#329This PR allows to update the following fields of a
kubernetes_job
resource without recreating it:active_deadline_seconds
backoff_limit
parallelism
It also forces the recreation of a job resource when its pod template
image
field is modified.Previously, it would detect a change was needed:
But submit an empty patch operation: