-
Notifications
You must be signed in to change notification settings - Fork 573
Elastic IP for VPC instances per issue 18 #65
Conversation
Ben, this looks great! Thanks so much. Some nitpicks:
After that, I'd be happy to merge, and we can work on some improvements. @johntdyer This patch doesn't allow for pre-allocated EIPs, but that would be an easy improvement to make after we merge this in! :) |
Great! Won't have that fix in until Tuesday or so. On Thu, May 2, 2013 at 2:51 PM, Mitchell Hashimoto
|
I've been trying out your branch here @bwhaley - you'll need to do something conditional with the launch_vpc_warning which still fires even when I've enabled an elastic_ip. Also a |
Thanks @jtopper, added a commit for that. |
No, that's true, I'm not. See the very first comment in this PR. Per @mitchellh above, he agreed to merge the request and fix that after integrating. OTOH, I'm happy to fix it if you can give me a quick pointer on how to track the EIP in the machine data dir. It wasn't obvious to me. |
Sorry, didn't notice that. The easiest way to get the EIP of an instance is to run |
I understand how to get the IP of the instance, but the part I wasn't clear |
I've had a good poke at the code and I can't see where useful machine state I think the existing provider probably just needs the 'id' serialized, and On 24 May 2013 14:50, bwhaley notifications@github.com wrote:
|
Nice work, I just tested it and this works perfectly for me. |
Great! Can this PR be merged now? |
@mitchellh We've been using this quite a bit over the last week with no problems - looks good to merge to me. |
+1 for merging this |
@@ -31,7 +31,7 @@ def read_ssh_info(aws, machine) | |||
|
|||
# Read the DNS info | |||
return { | |||
:host => server.dns_name || server.private_ip_address, | |||
:host => server.public_ip_address || server.dns_name || server.private_ip_address, |
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.
Is there a reason to make public_ip_address
default over dns_name
?
Or maybe we should just drop dns_name
altogether?
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.
No good reason. I just thought it made more sense to return an EIP if it was requested rather than a DNS name. We could potentially drop either one.
Also, private_ip_address
will only work if the user has some direct connection to an instance in a VPC, such as over a VPN.
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.
Yes private_ip_address
was added precisely for that.
Otherwise the PR looks good, I'll be merging it soon. Thanks.
Could you add the entry in |
+1 for feature merge |
+1 for merge. I needed this so I installed the vagrant-aws plugin built from @bwhaley 's fork! |
Elastic IP for VPC instances per issue 18
Great! One final comment on this request . You actually don't need a json file to store the ip . You can call the api to get this information - for reference - https://github.com/mitchellh/vagrant-aws/pull/129/files#L1R20 |
Yup :) and there are other things to cleanup, example doing a dissociate_address on publicip (it's done by Fog on terminate_instances). |
Is there a way to assign a previously allocated EIP using this provider, or anybody can suggest a workaround for this? |
You will need modifications like those made by a friend of mine here: https://github.com/grosendorf/knife-ec2/commits/master |
You could use this 9a86a91 |
We are happy to see vagrant-aws already have this cool feature! |
+1 for @yexingok request to update README.md file with this new feature! Was excited to see feature working (thank you!) but took a little while googling before I found this thread... :) Cheers! |
This is a README followup to mitchellh#65
I needed this functionality so I wrote up part of what you discussed in Issue #18
Done
Done
Done
Not done. Not sure what the best way to accomplish this is.Added in bwhaley/vagrant-aws@69a1f4f