-
Notifications
You must be signed in to change notification settings - Fork 79
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
Converged (a bit) to azure-arm coding style, added sysprep_done flag … #161
Conversation
…to indicate sysprep already been carried on
@sylviamoss Excuse me for referencing you directly but... what do you think about this PR? :-) |
@nywilken Sorry for referencing you directly, but ... seems my PR is completely ignored? :-/ |
Hi @flixman no need for apologies and thanks for the ping. We've been a bit heads down on some Packer core related changes so we are working through the pending PRs for each of the plugins with a bit of delay. So our apologies there. I wanted to review your PR on Friday but just got caught up with a bug for Azure. I will be taking a look as I would like to try and pull in your refactor before making the fix I have planned. Thanks for the contribution. |
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.
Overall this looks good. Thanks for the refactor and contribution.
I left one suggestion but this should be otherwise good to go. I'm running the acceptance tests. I will report back if there are any failures.
Hi @flixman thanks for making the update. It looks like you need to regenerate the HCL configuration after you latest change. You can fix this by running I tried testing this code and the code on main but I'm running into an issue, which I pasted below. I don't have too much experience with DTL so it might just be my setup. Can you confirm that you are able to build as expected using the updated code?
|
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.
@flixman thanks again for the contribution. This is good to go.
I committed two small changes to address the documentation generation and fix the failing tests. The test failure was due to a timeout issue; not with the changes in the code.
@nywilken Sorry for the radio silence: I have been catching fire for several months, and this felt off my mental stack :-/. Thank you!!! |
Sometimes, on win images, sysprep does not work as expected. The recommendation then is to perform the sysprep manually (removing the agent extensions, then calling sysprep, etc.), in which case packer can skip the sysprep. This PR provides that functionality and also performs some small refactoring to make the azure-arm and azure-dtl builders more alike.