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.

Sign up for GitHub

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

Jump to bottom

Add boot steps configuration option #103

Merged
merged 8 commits into from Nov 9, 2022
Merged

Add boot steps configuration option #103

merged 8 commits into from Nov 9, 2022

Conversation

jacob-carlborg
Copy link
Contributor

Adds a new configuration option called boot_steps. This is an array of tuples of boot commands, to type when the virtual machine is booted. The first element of the tuple is the actual boot command. The second element of the tuple is a description of what the boot command does. This is intended to be used for interactive installers that requires many commands to complete the installation.

Both the command and the description will be printed when logging is enabled. When debug mode is enabled Packer will pause after typing each boot command. This will make it easier to follow along the installation process and make sure the Packer and the installer are in sync.

Closes hashicorp/packer-plugin-sdk#216
Closes hashicorp/packer#11286

@jacob-carlborg
Add boot steps configuration option
628c457 
Adds a new configuration option called `boot_steps`. This is an array
of tuples of boot commands, to type when the virtual machine is
booted. The first element of the tuple is the actual boot command.
The second element of the tuple is a description of what the boot
command does. This is intended to be used for interactive installers
that requires many commands to complete the installation.

Both the command and the description will be printed when logging is
enabled. When debug mode is enabled Packer will pause after typing
each boot command. This will make it easier to follow along the
installation process and make sure the Packer and the installer are
in sync.

Closes hashicorp/packer#11824
Closes hashicorp/packer#11286
@jacob-carlborg jacob-carlborg requested a review from a team as a code owner October 23, 2022 13:06
@hashicorp-cla
Copy link

hashicorp-cla commented Oct 23, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@jacob-carlborg
Copy link
Contributor Author

  • I'm not entirely happy with the name boot_steps. Other suggestions are welcome
  • I couldn't find any existing test for boot_command to look at

lbajolet-hashicorp
lbajolet-hashicorp reviewed Oct 24, 2022
View reviewed changes
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hey @jacob-carlborg,

First off thanks for the PR, I like the proposal, and I think it brings value to the boot sequences, also very sorry for missing the issues that you opened earlier in the year, thanks for taking the time to implement them.

I do have a few comments on the code itself, feel free to address them, and we can do another pass later.

On another note, I wonder if this is the right place for this feature: since the boot config is straight out of the SDK, I think this code might find a better home in this project, as every plugin will then benefit from this change.
@nywilken: what's your thoughts on that? Do you think this would benefit from bumping it to the SDK?

Thanks again for the PR @jacob-carlborg!

builder/qemu/config.go Show resolved Hide resolved
builder/qemu/config.go Outdated Show resolved Hide resolved
builder/qemu/builder.go Outdated Show resolved Hide resolved
@jacob-carlborg
Copy link
Contributor Author

jacob-carlborg commented Oct 24, 2022 •
edited

On another note, I wonder if this is the right place for this feature: since the boot config is straight out of the SDK, I think this code might find a better home in this project, as every plugin will then benefit from this change.
nywilken: what's your thoughts on that? Do you think this would benefit from bumping it to the SDK?

The definition of the boot command config is in the SDK and there seem to be some validations as well. But besides that, executing the boot command step occurs in this project.

Please let me know what you decide and I can move some of the code to the SDK.

@jacob-carlborg
Copy link
Contributor Author

Now when boot_command and boot_steps are mutually exclusive the error is not reported immediately, but instead when the step is executed. This is not ideal. I now think it would make more sense to move some of this code to the SDK and add the check in the Prepare function:

https://github.com/hashicorp/packer-plugin-sdk/blob/54a995c473e79fce8f30841100b6e021838d41c1/bootcommand/config.go#L186

@jacob-carlborg jacob-carlborg mentioned this pull request Oct 28, 2022
Closed
@jacob-carlborg
Copy link
Contributor Author

jacob-carlborg commented Oct 28, 2022 •
edited

@lbajolet-hashicorp I've now moved some of the code to the SDK e00cc19 and hashicorp/packer-plugin-sdk#139. I don't know how you're handling dependent pull requests, but this will fail until the SDK PR is merged.

@jacob-carlborg jacob-carlborg force-pushed the boot-steps branch 2 times, most recently from e1fd321 to 322ce49 Compare November 7, 2022 20:32
@jacob-carlborg
Copy link
Contributor Author

@lbajolet-hashicorp I've updated the PR and removed the dependency on the SDK PR hashicorp/packer-plugin-sdk#139.

lbajolet-hashicorp
lbajolet-hashicorp reviewed Nov 8, 2022
View reviewed changes
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @jacob-carlborg,

The code looks good to me, I left just a few comments, which once addressed should lead to this being merged.

Thanks again for your patience and for the contribution, much appreciated!

builder/qemu/config.go Outdated Show resolved Hide resolved
builder/qemu/step_type_boot_command.go Outdated Show resolved Hide resolved
@jacob-carlborg
Copy link
Contributor Author

@lbajolet-hashicorp I've addressed all the comments now.

lbajolet-hashicorp
lbajolet-hashicorp approved these changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @jacob-carlborg,

Code looks good to me, we can merge this and it'll be included in the upcoming version 1.0.7 of the plugin.

Thanks again!

@lbajolet-hashicorp lbajolet-hashicorp merged commit 82fd7e2 into hashicorp:main Nov 9, 2022
@jacob-carlborg
Copy link
Contributor Author

Awesome, thanks.

@jacob-carlborg jacob-carlborg deleted the boot-steps branch November 10, 2022 06:33
@jacob-carlborg jacob-carlborg mentioned this pull request Nov 15, 2022
Closed
@jacob-carlborg jacob-carlborg mentioned this pull request Dec 10, 2022
Open
@jacob-carlborg jacob-carlborg mentioned this pull request Feb 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbajolet-hashicorp lbajolet-hashicorp lbajolet-hashicorp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Description of boot commands Pause for each boot command in debug mode
3 participants
@jacob-carlborg @hashicorp-cla @lbajolet-hashicorp