Skip to content

Commit

Permalink
Fixing a small logic error
Browse files Browse the repository at this point in the history
  • Loading branch information
Sander van Harmelen committed Apr 11, 2015
1 parent 365251f commit 8c37a95
Showing 1 changed file with 6 additions and 7 deletions.
13 changes: 6 additions & 7 deletions builtin/providers/cloudstack/resource_cloudstack_network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,14 @@ func resourceCloudStackNetworkUpdate(d *schema.ResourceData, meta interface{}) e
// Create a new parameter struct
p := cs.Network.NewUpdateNetworkParams(d.Id())

// Check if the name or display text is changed
if d.HasChange("name") || d.HasChange("display_text") {
// Check if the name is changed
if d.HasChange("name") {
p.SetName(name)
}

// Compute/set the display text
displaytext := d.Get("display_text").(string)
if displaytext == "" {
displaytext = name
}
// Check if the display text is changed
if d.HasChange("display_text") {
p.SetDisplaytext(d.Get("display_text").(string))
}

// Check if the cidr is changed
Expand Down

3 comments on commit 8c37a95

@catsby
Copy link
Contributor

@catsby catsby commented on 8c37a95 Apr 13, 2015

Choose a reason for hiding this comment

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

Prior to this, it seems display_text was kept in-sync with the name if display_text was not set in the configuration. It seems that with this commit, they will not be kept in-sync if the name changes in that situation. Is that correct, and if so, is that intentional?

@svanharmelen
Copy link
Contributor

Choose a reason for hiding this comment

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

@catsby thanks for the review! In a way you are right, but before this commit the display_text was actually never updated at all. If you look at the "old" code again, you'll notice that the variable displaytext was set, but p.SetDisplaytext(...) was never called to actually set/update the display_text.

So this commit fixes that logic error which enables you to update the display_text properly, but your right that at the same time it also changes the previously intended behaviour where I meant to keep these in sync when no specific value was set for display_text.

So maybe updating the code to:

if d.HasChange("name") || d.HasChange("display_text") {
  p.SetName(name)

  // Compute/set the display text
  displaytext := d.Get("display_text").(string)
  if displaytext == "" {
    displaytext = name
  }
  p.SetDisplaytext(displaytext)
}

would give a more expected result when changing the name without having specified a display_text...

Will create a PR to update the code accordingly, thanks again...

@catsby
Copy link
Contributor

@catsby catsby commented on 8c37a95 Apr 13, 2015

Choose a reason for hiding this comment

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

you'll notice that the variable displaytext was set, but p.SetDisplaytext(...) was never called to actually set/update the display_text.

Ah, I did not noticed that, cool. Sounds good on the new PR 👍

Please sign in to comment.