feat: add windows_sysprep_text field
#359
Conversation
|
|
aidanleuck
changed the title
aidanleuck
changed the title
vsphere-clone Add windows_sysprep_text field
tenthirtyam
changed the title
vsphere-clone Add windows_sysprep_text fieldwindows_sysprep_text field
tenthirtyam
force-pushed
the
aidaleuc/add-windows-sysprep-text
branch
2 times, most recently
from
84f4867 to
0085fd3
Compare
Allows users to submit a custom Windows Sysprep file as text instead of as a file. Marks `windows_sysprep_file` as deprecated in favor of `windows_sysprep_text`. Ref: hashicorp#357
tenthirtyam
force-pushed
the
aidaleuc/add-windows-sysprep-text
branch
from
0085fd3 to
b06daaf
Compare
|
@nywilken I've made some slight updates and force-pushed the changes. |
nywilken
left a comment
nywilken
left a comment
There was a problem hiding this comment.
Overall this looks good. I left a question around validating that only one of the attributes are set. Also we should add some examples. @aidanleuck if you some thoughts on how you would test that would only benefit the PR. Looking at the code as it stands you can add a few unit tests to validate mutual exclusivity and that file contents get passed into properly. But it is not clear to me how to integrate with the mock.
| options_number = options_number + 1 | ||
| } | ||
| if c.WindowsSysPrepFile != "" { | ||
| warnings = append(warnings, "`windows_sysprep_file` is deprecated and will be removed in a future release. please use `windows_sysprep_text`.") |
There was a problem hiding this comment.
Do we need a mutual exclusivity check here to error if both windows_systprep_file and windows_sysprep_text are provide?
There was a problem hiding this comment.
Agree! Otherwise it conflicts.
There was a problem hiding this comment.
A mutual exclusivity check is already included here. Line 149 prints an error message if options_number > 1 which is true if both windows_sysprep_file and windows_sysprep_text are included. Do you want a more detailed error message for the scenario when both are included?
There was a problem hiding this comment.
Thanks for calling this out. I didn't notice that logic above.
| - `windows_sysprep_file` (string) - Supply your own sysprep.xml file to allow full control of the customization process out-of-band of vSphere. | ||
| - `windows_sysprep_file` (string) - Provide a sysprep.xml file to allow control of the customization process independent of vSphere. This option is deprecated, please use windows_sysprep_text. | ||
|
|
||
| - `windows_sysprep_text` (string) - Provide the text for the sysprep.xml content to allow control of the customization process independent of vSphere. |
There was a problem hiding this comment.
I feel like we need some usage examples here. The ability to use file() in HCL is a good example, HCL also supports heredocs but I suspect this would not be easy for a user running JSON templates. JSON templates are legacy but we should call out that this attribute is intended to be used with user variables or HCL expressions that can provide the file contents and not the raw XML.
What do you think?
nywilken
left a comment
nywilken
left a comment
There was a problem hiding this comment.
This looks good to me. I will merge once all goes green.
4 participants
Closes #357
Allows users to submit a custom Windows Sysprep file as text instead of as a file. #357 discusses why this PR is useful. windows_sysprep_file will be marked as deprecated in favor of windows_sysprep_text.
Note: I have tested this on my own Packer builds and it works fine. I noticed that there are not any tests around customization, but I am happy to write some if necessary.