-
Notifications
You must be signed in to change notification settings - Fork 101
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
Update to use fog-google v1 #180
Conversation
I'm currently running into the below error ...
I opened fog/fog-google#300 as I believe this might be a bug in the fog-google library. |
Example Vagrantfile used for testing is below. There are going to be quite a few code changes required to get this working.
|
With the above example Vagrantfile I can successfully run
Here are some notes for myself, so I can continue debugging this in the future. Debugging command ...
https://github.com/hashicorp/vagrant/blob/master/plugins/communicators/ssh/communicator.rb#L311 |
I fixed the SSH glitch with more Vagrantfile hacking. Now the "up", "ssh", and "destroy" commands can be executed successfully. Although the "destroy command does not remove the disk, not sure if this is the expected behaviour or not. Next up ... Write some Ruby code to get rid of the Vagrantfile hacks. :-) |
Updated Vagrantfile example used for testing ...
|
Remaining items to test/fix before this PR is ready.
|
Need PR fog/fog-google#304 to fix config options |
Need PR fog/fog-google#305 merged to fix config options |
The last feature I need to fix is the disk auto delete feature. This is not working. After running Notes for my future self when I work on this in the near future. https://github.com/fog/fog-google/blob/master/lib/fog/compute/google/models/disk.rb#L69 |
Hey @seanmalloy - first, thank you very much for taking on this work! It's been a long haul to get the fog client updated and I'm glad to see other projects starting to pick it up. You're correct that P12 is legacy and will likely be dropped at some point. However, it's still an option for users to create that type of key, so it probably makes sense to keep that feature around. I would definitely suggest making JSON key the default and even add documentation that P12 is on the outs to discourage any new users from going that route. Also, I'm very appreciative of this work. You have a standing invitation to join me and the team for lunch next time you make it to Seattle! Feel free to hit me up anytime at erjohnso[at]google[dot]com. |
@erjohnso sounds good I will also get the P12 key stuff tested to make sure it works. I think I tried it a few weeks back and it was not working. I'm hoping to have this PR ready for review in the near future. Most of the functionality seems to be working now. Just have a few loose ends to finish off. |
Waiting for the below pull request to be merged to fix the disk auto delete feature of "vagrant destroy". |
Awesome work Sean :) 👍 |
@erjohnso and @Temikus it looks like v1 of the fog-google library deprecated the option for using the P12 key type. From what I can tell we now have to use JSON keys instead. See this block of code for details ... https://github.com/fog/fog-google/blob/master/lib/fog/google/shared.rb#L60-L64 Here is the stack trace I get when try to use the
What are your thoughts on this? Can I remove the support for the P12 key? I have tested all of the other options, and everything seems to be working. I just need to do a few code clean ups before this PR is ready for review. |
The final count down ...
|
I created fog/fog-google/issues/308 to get a better understanding of how to get rid of the deprecation warning when adding instance to an instance group. Not 100% sure if this is a bug in the fog-google library or not. |
Created fog/fog-google/issues/309 requesting a new release of the fog-google gem. |
I cleaned up my commit messages. Everything seems to be working. The only remaining task is to remove the @erjohnso and @Temikus let me know if you have any feedback on this PR. It should be good enough for a review now. |
The fog-google v1 library dropped support for the P12 key. Now only JSON keys are supported.
I removed support for the @erjohnso and @Temikus I'm looking forward to feed back on this PR. |
As a bonus I've just pushed up a commit to add support for GCE labels. I hope you like it! |
I like bonus features! Thank you @seanmalloy! I don't know when @Temikus will have a chance to dig in. But I likely won't have a good chunk of time until this coming weekend due to travel and busy dayjob. |
@erjohnso I totally understand your time constraints. Thanks for the update! |
Hiya @seanmalloy, Ok, looked it over and it looks good to me. Probably fixed a few bugs along the way. 👍 Given that P12 will stop working, I think we'll want to bump this to 2.0.0 release. To get ready for that, could you also update the README and CHANGELOG? |
@erjohnso I updated the CHANGELOG. I believe I already made the required updates to the README. Let me know I missed anything. |
I'll work on the 2.0 release - thank you very much for this fantastic work @seanmalloy! |
Sorry, I lapsed off of GitHub for the last 2 months, just started looking into my notifications. @erjohnso I can cut a release and run some tests while I'm at it tomorrow. Or you're already on top of it? |
And huge thanks to Sean, this is great :) |
@Temikus - go for it! Thank you! |
@seanmalloy QQ since I want to make sure I'm not breaking something - why did you add a runtime dependency on ffi here and strict-locked the version? This is causing issues in latest version of Vagrant (2.0.3) since vagrant's vendored install requires ffi 1.9.23. EDIT: Let me open a bug for this. #183 |
I am attempting to update vagrant-google to use v1.2.0 of the
fog-google library. I have made some progress, but there is still
a lot of work to do.
Any feedback on this PR is welcome.