-
Notifications
You must be signed in to change notification settings - Fork 112
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
Vdc update #275
Vdc update #275
Conversation
Signed-off-by: Vaidotas Bauzys <[email protected]>
Fix ignoring reservations guarantee values Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[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.
Great to see covering that many issues.
First round from me.
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[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.
Some initial comments for now.
More review coming
vcd/resource_vcd_org_vdc.go
Outdated
Optional: true, | ||
ForceNew: true, | ||
Description: "Percentage of allocated memory resources guaranteed to vApps deployed in this VDC. For example, if this value is 0.75, then 75% of allocated resources are guaranteed. Required when AllocationModel is AllocationVApp or AllocationPool. Value defaults to 1.0 if the element is empty.", | ||
Type: schema.TypeString, |
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.
Why are we changing the type from Float to String?
This change breaks compatibility.
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 read explanation I did for Dainius above.
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 explanation above is about the .ID field.
I am talking about changing the type of a field that will break existing Terraform scripts.
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.
Copying here:
in terraform file nothing changed: memory_guaranteed = 0.5 you still can write like this.
This change is must cause d.GetOk for empty value return 0 and d.Get for type float64 return false. And value can be not provided, 0 or other value.
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 one changed
vcd/resource_vcd_org_vdc.go
Outdated
|
||
adminVdc, err := adminOrg.GetAdminVdcByName(vdcName) | ||
if err != nil || adminVdc == (govcd.AdminVdc{}) { | ||
log.Printf("[DEBUG] Unable to find vDC.") |
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.
"vDC" -> "VDC" (also in the rest of the PR)
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.
Fixed through file
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[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.
My minor comment about adding a note about ID to changelog. Other than that I'm good. Test suite passes.
Thanks for sorting out the Float thing.
Signed-off-by: Vaidotas Bauzys <[email protected]>
CHANGELOG.md
Outdated
|
||
IMPROVEMENTS: | ||
* Fix ignoring of resource guarantee values in `vcd_org_vdc` [GH-265] | ||
* **Resource:** Org VDC `vcd_org_vdc` ID changed from name to vCD ID - [GH-275] |
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.
**Resource:**
highlight shouldn't be needed here. Unless I missed it, in guidelines there's no emphasis on improvements. They suggest using a form like this instead:
IMPROVEMENTS:
* resource/load_balancer: Add `ATTRIBUTE` argument (support X new functionality) [GH-12]
* resource/subnet: Now better [GH-22, GH-32]
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.
fixed
Signed-off-by: Vaidotas Bauzys <[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.
LGTM
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[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.
LGTM now!
Ref:
https://github.com/terraform-providers/terraform-provider-vcd/issues/222
https://github.com/terraform-providers/terraform-provider-vcd/issues/239
https://github.com/terraform-providers/terraform-provider-vcd/issues/237
https://github.com/terraform-providers/terraform-provider-vcd/issues/265
These changes fix bug where resources guarantee were ignored on serializing, allows Org VDC update to be udpated and read fills and refresh all data in state file, state ID changed from name to vCD resource ID.