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

only use vApp properties that are UserConfigurable #751

Merged
merged 2 commits into from
May 22, 2019
Merged

only use vApp properties that are UserConfigurable #751

merged 2 commits into from
May 22, 2019

Conversation

adarobin
Copy link
Contributor

This PR resolves #394 by ignoring vApp attributes that have UserConfigurable set to False.

Signed-off-by: Adam Robinson <adarobin@umich.edu>
@ghost ghost added the size/s Relative Sizing: Small label Apr 18, 2019
@bill-rich
Copy link
Contributor

Thanks @adarobin. This looks really great. Would it be possible to get an error thrown when a configuration attempts to set a non user configurable property? That way there won't be any confusion on why properties are being ignored.

@adarobin
Copy link
Contributor Author

adarobin commented May 6, 2019

@bill-rich Not opposed to adding a message, but I'm not sure how to handle that. If you look at #394, properties that are not user configurable are getting pulled in regardless of if they are actually in the Terraform code or not.

For example, if I attempt to deploy a Nested ESXi OVA without this patch the deployment will fail as the vApp contains the property guestinfo.debug with userConfigurable set to false. This happens with or without guestinfo.debug being declared in Terraform code. This is because Terraform is copying all of the existing vApp properties from the source VM into the ConfigSpec without regard to if they are userConfigurable or not.

I worked around this by ignoring all properties with userConfigurable set to false.

@adarobin
Copy link
Contributor Author

adarobin commented May 6, 2019

@bill-rich My brain isn't fully working on a Monday. It looks like the existing error handling that is right after this change is already taking care of this. If I try setting guestinfo.debug in my code I get thrown this error:


1 error(s) occurred:

* module.adarobin-dev-cluster.vsphere_virtual_machine.esxi: 1 error(s) occurred:

* vsphere_virtual_machine.esxi: error reconfiguring virtual machine: error in virtual machine configuration: unsupported vApp properties in vapp.properties: [guestinfo.debug]

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

@adarobin
Copy link
Contributor Author

adarobin commented May 6, 2019

Just pushed code that should do what you are asking. This is what I see if I specify guestinfo.debug in my code now:


1 error(s) occurred:

* module.adarobin-dev-cluster.vsphere_virtual_machine.esxi: 1 error(s) occurred:

* vsphere_virtual_machine.esxi: error reconfiguring virtual machine: error in virtual machine configuration: vApp property with userConfigurable=false specified in vapp.properties: [guestinfo.debug]

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.```

@adarobin
Copy link
Contributor Author

@bill-rich thoughts on the latest code?

Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

Thanks for adding in the error message. This looks ready to merge!

@bill-rich bill-rich merged commit 1891efa into hashicorp:master May 22, 2019
@adarobin adarobin deleted the fix-issue-394 branch May 23, 2019 16:09
@ghost ghost locked and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/s Relative Sizing: Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't touch vapp properties (ovf) where ovf:userConfigurable="false"
2 participants