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

Post-Processor: added vagrantfile_template_content option to allow for dynamic Vagrantfile content #112

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VishnuJin
Copy link
Contributor

As specified in the issue, users can now use templatefile function for dynamic content in Vagrantfile template

Example:
A vagrant template file

Vagrant.configure("2") do |config|
  config.ssh.private_key_path = ${my_ssh_key}
  config.ssh.forward_agent = true
end

A packer file with vagrant post processor

locals {
   vagrantfile = templatefile("vagrantfile.pkrtpl.hcl", { 
      my_ssh_key = "~/.ssh/rsa"
    })
 }
 ....
 post-processor "vagrant" {
    vagrantfile_template_content = local.vagrantfile
 } 

Resulting Vagrantfile inside Box file


# The contents below were provided by the Packer Vagrant post-processor

Vagrant.configure("2") do |config|
	config.vm.provider :docker do |docker, override|
		docker.image = "sha256:47045c52cefdfd009eb4cfb69dc7c2ac3b2af7808abb218164a7a2264847c939"
	end
end


# The contents below (if any) are custom contents provided by the
# Packer template during image build.
Vagrant.configure("2") do |config|
  config.ssh.private_key_path = ~/.ssh/rsa
  config.ssh.forward_agent = true
end

Closes #104

@VishnuJin VishnuJin requested a review from a team as a code owner February 3, 2024 14:09
Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Hi there apologies for the delayed review here. Our focus has been on Packer itself trying to resolve issues with plugin loading. Looking at the changes and reference issue I can understand the need for this feature.

However, I am a little unclear on the expected usage of vagrantfile_template and vagrantfile_template_content. Please take a look at my suggested changes and questions.

If you're no longer interested in making the change I do understand given the time to review. I'll label the PR and await your response.

if err != nil {
return nil, false, err

var templateContent string
Copy link
Member

Choose a reason for hiding this comment

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

I believe this condition might be nested in a conflicting if statement.

// lets default to using VagrantTemplateContent
templateContent := config.VagrantfileTemplateContent
if templateContent == "" && config.VagrantfileTemplate != "" {
	ui.Message(fmt.Sprintf("Using custom Vagrantfile: %s", config.VagrantfileTemplate))
	customBytes, err := ioutil.ReadFile(config.VagrantfileTemplate)
	if err != nil {
		return nil, false, err
	}
	templateContent = string(customBytes)
}
	
if templateContent != "" {
       customVagrantfile, err = interpolate.Render(templateContent, &config.ctx)
	if err != nil {
		return nil, false, err
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nywilken could you please see the below comment #112 (comment) and let me know if this is still needs to be changed !!

Comment on lines +304 to +307
if c.VagrantfileTemplateContent != "" && c.VagrantfileTemplate == "" {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(
"'vagrantfile_template' should be provided to use 'vagrantfile_template_content'"))
}
Copy link
Member

@nywilken nywilken Mar 19, 2024

Choose a reason for hiding this comment

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

I am of the understanding that you would want to specify just the content with no filepath needed. Is that not the case?

If they are mutually exclusive I would rewrite the logic to be as follows:

Suggested change
if c.VagrantfileTemplateContent != "" && c.VagrantfileTemplate == "" {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(
"'vagrantfile_template' should be provided to use 'vagrantfile_template_content'"))
}
if c.VagrantfileTemplateContent != "" && c.VagrantfileTemplate != "" {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(
"'vagrantfile_template' can not be used with 'vagrantfile_template_content'"))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nywilken if you see the example here these are Not Exclusive we are trying to load the value for the variable my_ssh_key in the vagrant_template

@nywilken nywilken added enhancement stage/waiting-reply stage/accepted Confirmed, and intend to work on. No timeline commitment though. labels Mar 19, 2024
@nywilken
Copy link
Member

Hi @VishnuJin just want to follow up to see if you would like to continue with this change. Please let me know if you have time to continue pushing it forward and if you have any questions about the provided feedback.

Thank you for your contributions to this plugin.

@VishnuJin VishnuJin requested a review from nywilken April 21, 2024 05:56
@nywilken nywilken removed their request for review September 13, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement stage/accepted Confirmed, and intend to work on. No timeline commitment though. stage/waiting-reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying content for vagrantfile_template
2 participants