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

Computername fix #334

merged 30 commits into from
Sep 24, 2019

Conversation

vbauzys
Copy link
Contributor

@vbauzys vbauzys commented Sep 20, 2019

Ref: https://github.com/terraform-providers/terraform-provider-vcd/issues/273

This is fix for an issue. Overall decision was made to introduce new parameter computer_name which explicitly sets computer name for VM. Existing implementation was left for back compatibility where computer name was set using name, but only when initscript was provided.
Also to help our customer warning message was added.
Note: Read haven't be added as Giuseppe is working on read, datasource and import capabilities.

vbauzys added 18 commits July 15, 2019 15:39
Signed-off-by: Vaidotas Bauzys <[email protected]>
Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd
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]>
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Some questions for now.

vcd/resource_vcd_vapp_vm.go Show resolved Hide resolved
vcd/resource_vcd_vapp_vm.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_vm_test.go Show resolved Hide resolved
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Only one question. So does it change the computer name automatically or is it needed to set customization.force = true

CHANGELOG.md Outdated
@@ -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]

CHANGELOG.md Show resolved Hide resolved
website/docs/r/vapp_vm.html.markdown Outdated Show resolved Hide resolved
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

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]>
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

LGTM now!

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]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Test suite and individual tests pass for me (the ones which were failing before)

Signed-off-by: Vaidotas Bauzys <[email protected]>
@ghost ghost added dependencies size/XL and removed size/M labels Sep 24, 2019
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

@lvirbalas
Copy link
Collaborator

The user reported an issue #338 with this implementation which is being fixed in #347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants