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

HTTPConfig : Add http_content to be served #43

Merged
merged 20 commits into from
Mar 23, 2021
Merged

Conversation

azr
Copy link
Contributor

@azr azr commented Mar 19, 2021

This will allow to

  • feature
  • docs
  • tests ; I also added some tests for the http config struct.

@azr azr added the enhancement New feature or request label Mar 19, 2021
@azr azr requested a review from a team as a code owner March 19, 2021 16:18
@azr azr marked this pull request as draft March 19, 2021 16:22
azr added 2 commits March 19, 2021 17:37
…before returning

In unit tests we would get before the server was up, causing failures
Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

Is this done? I made you some questions.
I haven't tested but the code looks good so far.

// `boot_command`. This is covered in more detail below. Example: Setting
// `"foo/bar"="baz", will allow you to http get on
// `http://{http_ip}:{http_port}/foo/bar`.
HTTPContent map[string]string `mapstructure:"http_content"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a backward incompatibility but it is something that will require all builder components using http_config to update their hcl2spec files in order to be compatible with this feature.
If I'm right, I have two questions:

  • Are you planning to write a blog post for this?
  • Should this be officially released on 1.8.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions!

Should this be officially released on 1.8.0?

The earlier we merge this one, the less manual updates we will have to do in plugins in order to support this. So my guess would be to merge this one now-ish.

Are you planning to write a blog post for this?

🤔 I wasn't planning on doing that ! But you are right !
This definitely could use a new guide or help section too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! We just need to remember to run generate on Packer when this one is merged. So that 1.7.1 will be compatible with these changes.

multistep/commonsteps/step_http_server.go Outdated Show resolved Hide resolved
@azr azr marked this pull request as ready for review March 22, 2021 14:00
Comment on lines 67 to 69
if sug := didyoumean.NameSuggestion(path, paths); sug != "" {
err += fmt.Sprintf("Did you mean %q?", sug)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes lives easier <3 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

( even saved me a few times in tests mode already )

Copy link
Contributor

Choose a reason for hiding this comment

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

this is super cool :D

@azr
Copy link
Contributor Author

azr commented Mar 22, 2021

This is ready for review 🙂

@SwampDragons
Copy link
Contributor

I pulled this to try and test and I can't get it working; on at least on the QEMU builder it is trying to read from a port 0 which seems wrong. Can you give a more complete example?

multistep/commonsteps/http_config.go Outdated Show resolved Hide resolved
Comment on lines 67 to 69
if sug := didyoumean.NameSuggestion(path, paths); sug != "" {
err += fmt.Sprintf("Did you mean %q?", sug)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is super cool :D

@@ -70,6 +126,8 @@ func (s *StepHTTPServer) Run(ctx context.Context, state multistep.StateBag) mult
func (s *StepHTTPServer) Cleanup(multistep.StateBag) {
if s.l != nil {
// Close the listener so that the HTTP server stops
s.l.Close()
if err := s.l.Close(); err != nil {
log.Printf("Failed closing http server on port %d: %v", s.l.Port, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this error print to the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, this error should be temporary and won't prevent Packer from working. We check if ports are already open for new calls to Listen. Finally, when Packer terminates: the connection will be auto closed on most systems. Though on some system it could take some time and cause a FS leak if it happened A LOT of times. Yup this should be at least be shown in the UI.

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

LGTM!
Tested with virtualbox-iso building and alpine image and it worked good! 👍🏼

@@ -0,0 +1,24 @@
package didyoumean
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the package name!

@azr azr merged commit 5a7ab7f into main Mar 23, 2021
@azr azr deleted the azr_http_serve_content branch March 23, 2021 11:17
azr added a commit to hashicorp/packer that referenced this pull request Mar 23, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to hashicorp/packer-plugin-vsphere that referenced this pull request Apr 9, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to hashicorp/packer-plugin-virtualbox that referenced this pull request Apr 15, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to hashicorp/packer-plugin-vmware that referenced this pull request Apr 16, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
azr added a commit to hashicorp/packer-plugin-proxmox that referenced this pull request Apr 19, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to hashicorp/packer-plugin-qemu that referenced this pull request Apr 19, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
SwampDragons pushed a commit to hashicorp/packer-plugin-cloudstack that referenced this pull request Apr 19, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to Parallels/packer-plugin-parallels that referenced this pull request Apr 20, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
sylviamoss pushed a commit to hashicorp/packer-plugin-hyperv that referenced this pull request Apr 21, 2021
This imports hashicorp/packer-plugin-sdk#43

* code generate things
* update docs
* update guides
* update examples

We want to add a new guide.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants