search

fog / fog-xenserver

Module for the 'fog' gem to support XENSERVER
MIT License
16 stars 22 forks source link

Change revert to revert_to #41

Closed israelriibeiro closed 9 years ago

israelriibeiro commented 9 years ago

I noticed that in server.rb(1), line 168, you used revert_to_vm( alias_method to revert_to_server), but the request in xen_server.rb(2) was revert_server. Change it, because you said that revert will be deprecate.

https://github.com/fog/fog-xenserver/blob/master/lib/fog/compute/xen_server/models/server.rb#L168 https://github.com/fog/fog-xenserver/blob/master/lib/fog/compute/xen_server.rb#L283

plribeiro3000 commented 9 years ago

@israelriibeiro We need to keep compatibility between the old api. See here: https://github.com/fog/fog/blob/master/lib/fog/xenserver/models/compute/server.rb#L242

Also, you just renamed the require, but not the file name.

fernandes commented 9 years ago

@plribeiro3000 I explained to @israelriibeiro about api compatibility, you're totally right

The request on XenServer is Vm.revert, but checking the server.rb I got you deprecated revert in favor of revert_to, any special reason why use revert_to instead of xenserver default request?

ty

fernandes commented 9 years ago

about renaming the require but not the file, exists both revert_server.rb and revert_to_server.rb requests, he should have added revert_to instead of replacing the line

plribeiro3000 commented 9 years ago

I switched it in the beggining of the extraction from the main fog gem. I can't really remember why but i think we should move to the Xenserver api and deprecate if necessary. This codebase is at beta so don't worry about breaking apis within.

fernandes commented 9 years ago

great... we are going to handle all this, keeping the compatibility with xenserver api

plribeiro3000 commented 9 years ago

Closing due inactivity. Comment in here if you are willing to play with this again.

israelriibeiro commented 9 years ago

Hi, @plribeiro3000 . If i keep compatibility would it be correct? If you say yes, i'll correct it now.

plribeiro3000 commented 9 years ago

Yes. But don't worry about compatibility now. Just make it be like XENSERVER api. Use revert instead of revert_to.

But i guess you will need to update more then just this file.

israelriibeiro commented 9 years ago

@plribeiro3000 , i tested using my master, which is without revert_to_server in requests. I found this: irb(main):014:0> vm.revert conn.snapshots.get_by_name('teste_snapshot').reference [fog][DEPRECATION] This method is DEPRECATED. Use #revert_to instead. NoMethodError: undefined method revert_to_vm' for #<Fog::Compute::XenServer::Real:0x007fbb03c74c80> from /Users/zertico/Documents/fog-xenserver/lib/fog/compute/xen_server/models/server.rb:168:inrevert_to' from /Users/zertico/Documents/fog-xenserver/lib/fog/compute/xen_server/models/server.rb:163:in revert' from (irb):14 from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/cli/console.rb:38:inrun' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/cli.rb:287:in console' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/vendor/thor/command.rb:27:inrun' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/vendor/thor/invocation.rb:121:in invoke_command' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/vendor/thor.rb:363:indispatch' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/vendor/thor/base.rb:440:in start' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/cli.rb:9:instart' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/bin/bundle:20:in block in <top (required)>' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/lib/bundler/friendly_errors.rb:5:inwith_friendly_errors' from /opt/rubies/2.1.2/lib/ruby/gems/2.1.0/gems/bundler-1.6.3/bin/bundle:20:in <top (required)>' from /opt/boxen/rbenv/versions/2.1.2/bin/bundle:23:inload' from /opt/boxen/rbenv/versions/2.1.2/bin/bundle:23:in `

'

I re-added revert to requests and manage to revert a snapshot. But, like you said, maybe i need to change more than that. Maybe, i think, it would be easier to open an issue. If you could guide me on this, i'll apreciated.

plribeiro3000 commented 9 years ago

@israelriibeiro You gonna have to:

I just found the issue related here. 093788035655d62c3d8ecf705da046c8288b7732 is introducing the request revert when a revert_to were already created and tested.

You gonna have to remove those tests as well.

I think this might be all.

plribeiro3000 commented 9 years ago

@israelriibeiro Just let me know when this is ready. This pr is floating for a long time already.

plribeiro3000 commented 9 years ago

@israelriibeiro Thank you for your interest in doing this but this issue was already solved at the version 0.1.2.