-
Notifications
You must be signed in to change notification settings - Fork 244
[main] Fix inconsistent plan issue with EKS launch template #1639
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
Conversation
95b4390
to
5398294
Compare
Please sign your commits with a verified key. |
if in.ID != nil && len(*in.ID) > 0 { | ||
obj["id"] = *in.ID | ||
} | ||
if in.Name != nil && len(*in.Name) > 0 { |
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.
Since Name and Version are required, is it possible for them to be nil here? What happens if you try to leave them empty?
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.
they should always be set, but I left the nil checks in just to be safe in case Rancher ever returns them unset. If we’re confident they’ll always come back populated, I can strip those out too, but I figured better to be defensive.
obj := map[string]interface{}{} | ||
if len(p) != 0 && p[0] != nil { | ||
obj = p[0].(map[string]interface{}) | ||
} |
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 is a weird one.
What is the result of removing this? Isn't 'p' the value of the plan at this point?
If so, I would expect this to affect the create function.
I also see that this is the only argument that potentially updates the plan, so maybe this was a bug all along?
This also eliminates 'p' in the function, so we should remove that argument and remove the 'v' in the parent function, right?
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.
Yeah, that makes sense. looks like pulling values from p was what caused the plan/state mismatch we were seeing with the version. Since we’ve marked id, name, and version as required now, I agree we don’t need to merge anything from the plan, so I’ll clean that up and remove the p argument from both the function and the parent call.
Targeting v9 and v8 per #730 |
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.
Please use one of the following prefixes: "fix:", "feature:", "feat:", "refactor!:", "feature!:", or "feat!:".
This enables release-please to automatically determine the type of release (major, minor, patch) based on the commit message.
update the condition to check for nil in version
3d3563f
to
25372da
Compare
@mjura I have updated the message. |
25372da
to
aff905f
Compare
(cherry picked from commit 3140be6)
(cherry picked from commit 3140be6)
Co-authored-by: Krunal Hingu <[email protected]>
Issue:
#730
Problem
The Rancher2 provider does not reconcile computed launch template version values properly during Terraform runs. When the launch template version changes (e.g., due to new AMI or configuration updates), Terraform’s plan phase uses a default or stale version (0), but the apply phase sees the actual updated version (e.g., 4). This mismatch causes the provider to report an invalid new value error and fail the apply.
Solution
The issue was resolved by updating the Rancher2 Terraform provider to mark the
launch_template.version
field ascomputed = true
and removing the default value (default = 1
).This change ensures that Terraform correctly waits for the actual launch template version value to be known during the apply phase, instead of relying on a hardcoded default or outdated value from the plan phase.
With this fix, the provider can now handle dynamic version changes from the launch template (such as when a new version is triggered by configuration changes), preventing inconsistent plans and apply failures.
Testing
Manual Testing
Automated Testing
No automated tests were added, as the fix primarily addresses a runtime inconsistency in Terraform provider behavior rather than a logic bug.
QA Testing Considerations
Regressions Considerations