-
Notifications
You must be signed in to change notification settings - Fork 260
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
Docs update and custom image options update #538
Docs update and custom image options update #538
Conversation
Signed-off-by: Noel Georgi <[email protected]>
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 picking this up! I had meant to add these here after doing so in godo, but it seems to have slipped my mind.
It would be great to add an additional step to this test exercising the update method:
func TestAccDigitalOceanCustomImageFull(t *testing.T) { |
_, _, err := client.Images.Update(ctx, id, &godo.ImageUpdateRequest{ | ||
Name: name, | ||
}) | ||
if d.HasChanges("name", "description", "distribution") { |
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.
👀 Hadn't noticed they've introduced HasChanges
. Much nicer than the old if d.HasChange("foo") || d.HasChange("bar")
syntaxt.
imageUpdateRequest := &godo.ImageUpdateRequest{} | ||
|
||
if d.HasChange("name") { | ||
imageUpdateRequest.Name = imageName | ||
} | ||
if d.HasChange("distribution") { | ||
imageDistribution := d.Get("distribution").(string) | ||
imageUpdateRequest.Distribution = imageDistribution | ||
} | ||
|
||
if d.HasChange("description") { | ||
imageDescription := d.Get("description").(string) | ||
imageUpdateRequest.Description = imageDescription | ||
} |
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.
imageUpdateRequest := &godo.ImageUpdateRequest{} | |
if d.HasChange("name") { | |
imageUpdateRequest.Name = imageName | |
} | |
if d.HasChange("distribution") { | |
imageDistribution := d.Get("distribution").(string) | |
imageUpdateRequest.Distribution = imageDistribution | |
} | |
if d.HasChange("description") { | |
imageDescription := d.Get("description").(string) | |
imageUpdateRequest.Description = imageDescription | |
} | |
imageUpdateRequest := &godo.ImageUpdateRequest{ | |
Name: imageName, | |
Distribution: d.Get("distribution").(string), | |
Description: d.Get("description").(string), | |
} | |
We can simplify this a bit. PUT is idempotent, so no reason not to build the full request.
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 didn't noticed it was idempotent, was trying to change only fields that changed. I'll make this change
@andrewsomething I'll add a test for |
@andrewsomething I'm so sorry. I forgot about this. Thanks for taking it up 👍 ❤️ |
* Docs update and custom image options update support Signed-off-by: Noel Georgi <[email protected]> * Apply review suggestion. * Add an acceptance test for updates. Co-authored-by: Andrew Starr-Bochicchio <[email protected]>
Signed-off-by: Noel Georgi [email protected]
description
anddistribution
from images: Support updating distribution and description. godo#413