Skip to content
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

Computername fix #334

Merged
merged 30 commits into from
Sep 24, 2019
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6585559
Add missing file
vbauzys Jul 15, 2019
e247d25
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
vbauzys Jul 15, 2019
cf1e7c2
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Jul 23, 2019
be6790f
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
vbauzys Jul 24, 2019
6f4c5cb
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
vbauzys Jul 25, 2019
c73d402
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
vbauzys Jul 26, 2019
261d142
git push origin master
vbauzys Jul 29, 2019
c431d43
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Aug 1, 2019
414b3bf
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Aug 15, 2019
8b03ab8
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
vbauzys Aug 22, 2019
36c5487
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Sep 3, 2019
db01eec
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Sep 9, 2019
331e895
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Sep 12, 2019
a85d555
git push origin masterMerge branch 'master' of github.com:terraform-p…
vbauzys Sep 19, 2019
d7004b3
Add computer_name
vbauzys Sep 20, 2019
6d9cba1
Add documentation
vbauzys Sep 20, 2019
f2360e6
Add changelog
vbauzys Sep 20, 2019
1923d9c
Add test
vbauzys Sep 20, 2019
92ad8a5
Update description
vbauzys Sep 23, 2019
cada317
Added missing description
vbauzys Sep 23, 2019
ec66094
Add cover one missing case
vbauzys Sep 23, 2019
d828656
change warning message
vbauzys Sep 23, 2019
de3977f
Improve changelog
vbauzys Sep 23, 2019
34fbd10
Improve changelog
vbauzys Sep 23, 2019
4b41cf4
change from optional to computed
vbauzys Sep 23, 2019
cb4dff1
change from optional and computed
vbauzys Sep 24, 2019
15a3d8f
Improve test, not related to this PR
vbauzys Sep 24, 2019
ffc3573
Clean double function
vbauzys Sep 24, 2019
c2e1914
bump govcd version
vbauzys Sep 24, 2019
d63b7bb
Removed method which was moved to govcd
vbauzys Sep 24, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ IMPROVEMENTS:
* `vcd_org_vdc` Add import capability - [#324]
* `vcd_org_vdc` Add full read capability - [#324]
* Upgrade Terraform SDK dependency to 0.12.8 [#320]
* `resource/vcd_vapp_vm` have new field `computer_name` [#334]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `resource/vcd_vapp_vm` have new field `computer_name` [#334]
* `resource/vcd_vapp_vm` has new field `computer_name` [#334]


BUG FIXES:
Didainius marked this conversation as resolved.
Show resolved Hide resolved
* Change default value for `vcd_org.deployed_vm_quota` and `vcd_org.stored_vm_quota`. It was incorrectly set at `-1` instead of `0`.
Expand Down
45 changes: 40 additions & 5 deletions vcd/resource_vcd_vapp_vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ func resourceVcdVAppVm() *schema.Resource {
Required: true,
ForceNew: true,
},
"computer_name": &schema.Schema{
dataclouder marked this conversation as resolved.
Show resolved Hide resolved
Type: schema.TypeString,
Optional: true,
},
"org": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -88,6 +92,8 @@ func resourceVcdVAppVm() *schema.Resource {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: anyValueWarningValidator(true,
"With next version `name` as computer name won't be set together with `initscript`. Please use `computer_name`."),
dataclouder marked this conversation as resolved.
Show resolved Hide resolved
},
"metadata": {
Type: schema.TypeMap,
Expand Down Expand Up @@ -309,15 +315,32 @@ func resourceVcdVAppVmCreate(d *schema.ResourceData, meta interface{}) error {
}
}

// for back compatibility we allow to set computer name from `name` if computer_name isn't provided
var computerName string
if cName, ok := d.GetOk("computer_name"); ok {
computerName = cName.(string)
} else {
computerName = d.Get("name").(string)
}

if initScript, ok := d.GetOk("initscript"); ok {
task, err := vm.RunCustomizationScript(d.Get("name").(string), initScript.(string))
task, err := vm.RunCustomizationScript(computerName, initScript.(string))
if err != nil {
return fmt.Errorf("error with init script setting: %#v", err)
}
err = task.WaitTaskCompletion()
if err != nil {
return fmt.Errorf(errorCompletingTask, err)
}
} else {
task, err := vm.Customize(computerName, "", false)
if err != nil {
return fmt.Errorf("error with applying computer name: %#v", err)
}
err = task.WaitTaskCompletion()
if err != nil {
return fmt.Errorf(errorCompletingTask, err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is about the whole 318:343 approach. As it is right now, we're mixing the old behaviour with the new one, but I believe we should be prioritising new behaviour and leaving the old logic intact. In other words, we can simplify as follows:

  1. Leave old code intact, only add a warning message inside it:
	if initScript, ok := d.GetOk("initscript"); ok {
			task, err := vm.RunCustomizationScript(d.Get("name").(string), initScript.(string))
			if err != nil {
				return fmt.Errorf("error with init script setting: %#v", err)
			}
			err = task.WaitTaskCompletion()
			if err != nil {
				return fmt.Errorf(errorCompletingTask, err)
			}
  1. Remove "else" and just do this part if the new computer_name is defined:
	} else { // Remove and check for computer_name instead
			task, err := vm.Customize(computerName, "", false)
			if err != nil {
				return fmt.Errorf("error with applying computer name: %#v", err)
			}
			err = task.WaitTaskCompletion()
			if err != nil {
				return fmt.Errorf(errorCompletingTask, err)
			}

So, if the user defines the new computer_name we don't care about old way of setting it. If they don't, we use the old code without modifications and print a warning.

CC: @Didainius @dataclouder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget we need to leave initscript intact! And with your proposal we would overwrite initscript to "".

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, because:
"} else { // Remove and check for computer_name instead"
Idea is that you set computer_name only if it's defined. If it's undefined, you don't "overwrite" the old logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With current API, to change computer_name you have to provide initscript - what's why it's implemented as it is now. In general all logic is left - no previous behaviour is removed - it works as it was working.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @lvirbalas is right. The else block will always be triggered now and VMname will always be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, for this case I added change

}

if _, ok := d.GetOk("guest_properties"); ok {
Expand Down Expand Up @@ -554,11 +577,11 @@ func resourceVcdVAppVmUpdateExecute(d *schema.ResourceData, meta interface{}) er
}

if d.HasChange("memory") || d.HasChange("cpus") || d.HasChange("cpu_cores") || d.HasChange("power_on") || d.HasChange("disk") ||
d.HasChange("expose_hardware_virtualization") || d.HasChange("network") {
d.HasChange("expose_hardware_virtualization") || d.HasChange("network") || d.HasChange("computer_name") {

log.Printf("[TRACE] VM %s has changes: memory(%t), cpus(%t), cpu_cores(%t), power_on(%t), disk(%t), expose_hardware_virtualization(%t), network(%t)",
log.Printf("[TRACE] VM %s has changes: memory(%t), cpus(%t), cpu_cores(%t), power_on(%t), disk(%t), expose_hardware_virtualization(%t), network(%t), computer_name(%t)",
vm.VM.Name, d.HasChange("memory"), d.HasChange("cpus"), d.HasChange("cpu_cores"), d.HasChange("power_on"), d.HasChange("disk"),
d.HasChange("expose_hardware_virtualization"), d.HasChange("network"))
d.HasChange("expose_hardware_virtualization"), d.HasChange("network"), d.HasChange("computer_name"))

// If customization is not requested then a simple shutdown is enough
if vmStatusBeforeUpdate != "POWERED_OFF" && !customizationNeeded {
Expand Down Expand Up @@ -657,6 +680,18 @@ func resourceVcdVAppVmUpdateExecute(d *schema.ResourceData, meta interface{}) er
}
}

// we pass init script, to not override with empty one
if d.HasChange("computer_name") {
task, err := vm.Customize(d.Get("computer_name").(string), d.Get("initscript").(string), false)
if err != nil {
return fmt.Errorf("error with udpating computer name: %#v", err)
}
err = task.WaitTaskCompletion()
if err != nil {
return fmt.Errorf(errorCompletingTask, err)
}
}

}

// If the VM was powered off during update but it has to be powered off
Expand Down Expand Up @@ -803,7 +838,7 @@ func resourceVcdVAppVmRead(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("error getting VM : %#v", err)
}

d.Set("name", vm.VM.Name)
_ = d.Set("name", vm.VM.Name)

// Read either new or deprecated networks configuration based on which are used
switch {
Expand Down
4 changes: 4 additions & 0 deletions vcd/resource_vcd_vapp_vm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func TestAccVcdVAppVm_Basic(t *testing.T) {
"CatalogItem": testSuiteCatalogOVAItem,
"VappName": vappName2,
"VmName": vmName,
"ComputerName": vmName + "-unique",
"diskName": diskName,
"size": "5",
"busType": "SCSI",
Expand Down Expand Up @@ -54,6 +55,8 @@ func TestAccVcdVAppVm_Basic(t *testing.T) {
testAccCheckVcdVAppVmExists(vappName2, vmName, "vcd_vapp_vm."+vmName, &vapp, &vm),
resource.TestCheckResourceAttr(
"vcd_vapp_vm."+vmName, "name", vmName),
resource.TestCheckResourceAttr(
dataclouder marked this conversation as resolved.
Show resolved Hide resolved
"vcd_vapp_vm."+vmName, "computer_name", vmName+"-unique"),
resource.TestCheckResourceAttr(
"vcd_vapp_vm."+vmName, "ip", "10.10.102.161"),
resource.TestCheckResourceAttr(
Expand Down Expand Up @@ -108,6 +111,7 @@ resource "vcd_vapp_vm" "{{.VmName}}" {
vapp_name = "${vcd_vapp.{{.VappName}}.name}"
network_name = "${vcd_network_routed.{{.NetworkName}}.name}"
name = "{{.VmName}}"
computer_name = "{{.ComputerName}}"
catalog_name = "{{.Catalog}}"
template_name = "{{.CatalogItem}}"
memory = 1024
Expand Down
8 changes: 8 additions & 0 deletions vcd/suppressfuncs.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ func noopValueWarningValidator(fieldValue interface{}, warningText string) schem
}
}

// anyValueWarningValidator is a validator which only emits always warning string
func anyValueWarningValidator(fieldValue interface{}, warningText string) schema.SchemaValidateFunc {
return func(i interface{}, k string) (warnings []string, errors []error) {
warnings = append(warnings, fmt.Sprintf("%s\n\n", warningText))
return
}
}

func checkEmptyOrSingleIP() schema.SchemaValidateFunc {
return func(i interface{}, k string) (s []string, es []error) {
v, ok := i.(string)
Expand Down
3 changes: 2 additions & 1 deletion website/docs/r/vapp_vm.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ The following arguments are supported:
* `vdc` - (Optional; *v2.0+*) The name of VDC to use, optional if defined at provider level
* `vapp_name` - (Required) The vApp this VM should belong to.
* `name` - (Required) A unique name for the VM
* `computer_name` - (Optional; *v2.5+*) A unique name
dataclouder marked this conversation as resolved.
Show resolved Hide resolved
* `catalog_name` - (Required) The catalog name in which to find the given vApp Template
* `template_name` - (Required) The name of the vApp Template to use
* `memory` - (Optional) The amount of RAM (in MB) to allocate to the VM
Expand Down Expand Up @@ -252,4 +253,4 @@ resource "vcd_vapp_vm" "web2" {
force = false
}
}
```
```