-
Notifications
You must be signed in to change notification settings - Fork 265
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
Add support for Kubernetes maintenance policy #631
Add support for Kubernetes maintenance policy #631
Conversation
0e7d044
to
a8d56ab
Compare
@scotchneat Could you please have a look to see if this is moving in the right direction? Thanks. |
Hi @atombrella, thanks for your patience. I'll take a look at it shortly. |
@atombrella I just wanted to drop an update to let you know we haven't forgotten about your contribution. While reviewing it, I noticed some strange behavior with the tests which relate to the outdated SDK version of Terraform we're using. We're looking into this now and will likely have to wait for that to be resolved before proceeding with this change. |
Good :) I think there's still work that needs to be done, so I'm primarily hoping for a few hints on what I'm missing. In particular, regarding the unit tests. |
a8d56ab
to
36f8e49
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.
Thanks for working on this! I've left a few comments and suggestions in line.
In addition to those, you'll also need to make a change to the read function to set the policy returned by the API to state:
terraform-provider-digitalocean/digitalocean/resource_digitalocean_kubernetes_cluster.go
Line 290 in 2427940
func digitaloceanKubernetesClusterRead( |
You'll need to create a "flatten" function to do so. (More detail about what that means in one of the inline comments.) Something like this:
func flattenMaintPolicyOpts(opts *godo.KubernetesMaintenancePolicy) []map[string]interface{} {
result := make([]map[string]interface{}, 0)
item := make(map[string]interface{})
item["day"] = opts.Day.String()
item["duration"] = opts.Duration
item["start_time"] = opts.StartTime
result = append(result, item)
return result
}
@@ -345,6 +368,8 @@ func resourceDigitalOceanKubernetesClusterUpdate(ctx context.Context, d *schema. | |||
Tags: expandTags(d.Get("tags").(*schema.Set).List()), | |||
AutoUpgrade: godo.Bool(d.Get("auto_upgrade").(bool)), | |||
SurgeUpgrade: d.Get("surge_upgrade").(bool), | |||
|
|||
MaintenancePolicy: d.Get("maintenance_policy").(*godo.KubernetesMaintenancePolicy), |
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 also needs to call expandMaintPolicyOpts
.
Also, this line above will need to updated to for the addition:
terraform-provider-digitalocean/digitalocean/resource_digitalocean_kubernetes_cluster.go
Line 364 in 2427940
if d.HasChange("name") || d.HasChange("tags") || d.HasChange("auto_upgrade") || d.HasChange("surge_upgrade") { |
The SDK has actually grown a new method recently that can make this cleaner:
https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/helper/schema#ResourceData.HasChanges
So it can now look like:
if d.HasChanges("name", "tags", "auto_upgrade", "surge_upgrade", "maintenance_policy") {
2427940
to
5d8a632
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.
@andrewsomething Thanks a lot for the input. I'll work a bit more with it, and then ask for a second review, so it can be merged.
@atombrella Sounds good! Let us know if you run into any problems. |
5d8a632
to
3872ff1
Compare
@andrewsomething I opened an issue with a suggestion how to test the provider locally (and have used this trick 👍 ). I did a
So seems to work :) On a related note, I get a warning that the empty provider block in the |
Fixes digitalocean#607. Co-Authored-By: Andrew Starr-Bochicchio <[email protected]>
3872ff1
to
7d3cbd3
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.
👍 Looks great!
I did add an additional commit with some more testing and one addressing something I missed in my initial review. Specifically, maintenance_policy
needs to be both Optional
and Computed
. As it does not have a default value, it needs to be "computed" from the API if it is not explicitly set by the user.
Thanks again for working on this!
@andrewsomething Thanks :) I'm learning something, so that's a nice reward. |
I'm not too familiar with GoLang, but maybe this is a good opportunity to work a bit with it.