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

Add random hostname option #216

Merged
merged 3 commits into from Sep 4, 2014
Merged

Add random hostname option #216

merged 3 commits into from Sep 4, 2014

Conversation

nicot
Copy link
Contributor

@nicot nicot commented Jul 24, 2014

Let the user add information to the hostname from their Vagrantfile.

When on, prevent domain name collisions.

Off by default.

Fixes #215

@nicot nicot closed this Jul 24, 2014
@nicot nicot reopened this Jul 24, 2014
@@ -50,7 +49,8 @@ def build_domain_name(env)
config.default_prefix.to_s
end
domain_name.gsub!(/[^-a-z0-9_]/i, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

As this point domain_name is just the prefix, which is shared between all VMs in the Vagrantfile. After the gsub, you need

domain_name << '_'
domain_name << env[:machine].name.to_s

to add the individual machine's name.

@bradleyd
Copy link

This looks good. Add this to readme??

domain_name.gsub!(/[^-a-z0-9_]/i, '')
domain_name << "_#{postfix}"
domain_name << "_#{Time.now.utc.to_i}_#{SecureRandom.hex(10)}" if config.random_hostname
Copy link
Contributor

Choose a reason for hiding this comment

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

As this point domain_name is just the prefix, which is shared between all VMs in the Vagrantfile. Before appending the random string, you need

domain_name << '_'
domain_name << env[:machine].name.to_s

to add the individual machine's name.

@sciurus
Copy link
Contributor

sciurus commented Jul 25, 2014

Thanks for working on this, but I'm confused exactly what values you're intending to use in the name. Here's what I was expecting

prefix = config.default_prefix ? config.default_prefix.to_s : env[:root_path].basename.to_s
machine_name = env[:machine].name.to_s
postfix = config.random_hostname ? "_#{Time.now.utc.to_i}_#{SecureRandom.hex(10)}" : ''

domain_name = "#{prefix}_#{machine_name}#{postfix}

This commit lets users enable hostname randomization from their
Vagrantfile. Without this option enabled, domain creation will fail when
multiple VMs are spun up from the same Vagrantfile, due to a domain name
conflict.
@nicot
Copy link
Contributor Author

nicot commented Jul 31, 2014

@sciurus @purpleidea Is this what you had in mind?

@sciurus
Copy link
Contributor

sciurus commented Jul 31, 2014

Thanks @nicot ; name generation now looks good to me!

I think you'll need to update the "builds simple domain name test" case.

@purpleidea
Copy link
Contributor

@nicot didn't test, but this looks good :)

Assuming this passes the tests, +1

Thanks!

@pronix
Copy link
Member

pronix commented Aug 5, 2014

add please description to README

Nico Tonozzi and others added 2 commits August 5, 2014 09:16
Updated documentation so that users could find the new option.

Updated test case to behave as expected.
@sciurus
Copy link
Contributor

sciurus commented Aug 11, 2014

@nicot What I meant by updating the simple name test was something like this

 describe VagrantPlugins::ProviderLibvirt::Action::SetNameOfDomain do
   before :each do
     @env = EnvironmentHelper.new
+    @env.name = 'name'
   end

   it "builds unique domain name" do
@@ -16,6 +17,6 @@ describe VagrantPlugins::ProviderLibvirt::Action::SetNameOfDomain do
   it "builds simple domain name" do
     @env.default_prefix= 'pre'
     dmn = VagrantPlugins::ProviderLibvirt::Action::SetNameOfDomain.new(Object.new, @env)
-    dmn.build_domain_name(@env).should eq('pre_') 
+    dmn.build_domain_name(@env).should eq('pre_name')
   end
 end

so we test that the VM's name is incorporated in the domain name.

@purpleidea
Copy link
Contributor

@pronix @sciurus Shouldn't this be merged by now :P
My machine error-ed today with: "Too many random characters generated for VM's"...

@sciurus
Copy link
Contributor

sciurus commented Aug 28, 2014

I'm fine with merging this and then fixing up the tests, since I haven't heard from @nicot in a couple weeks.

@sciurus
Copy link
Contributor

sciurus commented Sep 3, 2014

@pronix can you please merge this and cut a release?

@nicot
Copy link
Contributor Author

nicot commented Sep 3, 2014

@sciurus I no longer have an environment where I can test these changes, so if you or anyone else wanted to take that over I'd appreciate it.

pronix added a commit that referenced this pull request Sep 4, 2014
Add random hostname option
@pronix pronix merged commit b3d888a into vagrant-libvirt:master Sep 4, 2014
@sciurus
Copy link
Contributor

sciurus commented Sep 4, 2014

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

This hostnames break when used with vagrant-hostmanager.
5 participants