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

[openstack] volume API v2 implementation #3730

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

dhague
Copy link
Contributor

@dhague dhague commented Oct 23, 2015

Create Fog::OpenStack::Volume::V1 and Fog::OpenStack::Volume::V2, both inherited from Fog::OpenStack::Volume class.
Adjust tests to pass against Kilo & re-record VCRs.
Test each of Fog::Volume::OpenStack, Fog::Volume::OpenStack::V1 and Fog::Volume::OpenStack::V2.

@dhague dhague force-pushed the openstack_volume_v2_api branch 4 times, most recently from fe9ffe6 to 99871e8 Compare October 23, 2015 15:42
@dhague
Copy link
Contributor Author

dhague commented Oct 26, 2015

This build failure makes no sense. It passes for jRuby, but not for conventional rubies. Failures are of the form:

        /home/travis/.rvm/gems/ruby-2.2.0/gems/fog-core-1.32.1/lib/fog/core/services_mixin.rb:18:in `rescue in new'
        /home/travis/.rvm/gems/ruby-2.2.0/gems/fog-core-1.32.1/lib/fog/core/services_mixin.rb:8:in `new'
        /home/travis/.rvm/gems/ruby-2.2.0/gems/fog-core-1.32.1/lib/fog/core/services_mixin.rb:4:in `[]'
        tests/openstack/requests/volume/availability_zone_tests.rb:10:in `block (3 levels) in <top (required)>'

The call from the test (availability_zone_tests.rb:10) is:

      Fog::Volume[:openstack].list_zones.body

and the services_mixin code is:

 1 module Fog
 2   module ServicesMixin
 3     def [](provider)
 4       new(:provider => provider)
 5     end
 6 
 7     def new(attributes)
 8       attributes    = attributes.dup # Prevent delete from having side effects
 9       provider      = attributes.delete(:provider).to_s.downcase.to_sym
10       provider_name = Fog.providers[provider]
11
12       raise ArgumentError, "#{provider} is not a recognized provider" unless providers.include?(provider)
13
14       require_service_provider_library(service_name.downcase, provider)
15       spc = service_provider_constant(service_name, provider_name)
16       spc.new(attributes)
17     rescue LoadError, NameError  # Only rescue errors in finding the libraries, allow connection errors through to the caller
18       raise ArgumentError, "#{provider} has no #{service_name.downcase} service"
19     end

At line 4, provider is :openstack, so we are passing new() a Hash of {:provider => :openstack}.
This becomes the attributes parameter used at line 8, which is where the ArgumentError is thrown, apparently from calling dup() (inherited from Object).

Needless to say, this all works fine on my machine - so it somehow looks like Travis is using broken versions of Ruby 2.x?

@Carthaca
Copy link
Contributor

@dhague: I want to make you aware that this includes the fix for #3618, meaning it is breaking backward compatibility - maybe you should leave this change out

@dhague
Copy link
Contributor Author

dhague commented Oct 29, 2015

We are in a period of breaking changes anyway, as we move towards Fog 2.0 - so a breaking change that fixes a bug, I am OK with :-)

What do others think?

@plribeiro3000
Copy link
Member

@dhague Agree. but just to be clear we are in the same page 2.0.0 is out already.

@dhague
Copy link
Contributor Author

dhague commented Oct 30, 2015

@plribeiro3000 That's confusing - now I look at https://github.com/fog/fog/releases/tag/v1.35.0 I see it in the comments - but I am confused as to why this release is not tagged as v2.0.0. Haven't we broken semantic versioning (http://semver.org/) with this approach?
[edit] I see there is a release v2.0.0.pre.0 (https://github.com/fog/fog/releases/tag/v2.0.0.pre.0) - but that looks like a pre-release of 2.0.0, not an official one.

@plribeiro3000
Copy link
Member

@dhague Alright. So we might not have released 2.0.0 yet.
The version 1.35.0 is basically because of DigitalOcean. The Pull request only got done after 2.0.0.pre.0 and if we did not include that change in 1.x.x series fog would just stop working for it.
The concern with the 1.x.x series is that there is a lot of distros that still maintain and use 1.9.3.
So if we did not had this change in 1.x.x it would just not work for those guys anymore.

T.L.D.R: We might have breaked semver and possibly will need again to release the last version with constraints in the dependencies but that was the best path of course taking all the requirements. 😅

@dhague
Copy link
Contributor Author

dhague commented Oct 30, 2015

OK, just wanted to make sure I hadn't missed the 2.0.0 release train :)

@plribeiro3000
Copy link
Member

👍

Create Fog::OpenStack::Volume::V1 and Fog::OpenStack::Volume::V2, both inherited from Fog::OpenStack::Volume class. Adjust tests to pass against Kilo & re-record VCRs.
Test each of Fog::Volume::OpenStack, Fog::Volume::OpenStack::V1 and Fog::Volume::OpenStack::V2
@Ladas
Copy link
Contributor

Ladas commented Nov 18, 2015

@dhague breaking change is fine with me, when it brings Cinder V2 :-)

Code looks good to me, though I haven't tested it. I should get to testing in coming weeks, I am fine with merging it and fixing possible minor bugs later. :-)

Good job 👍

@dhague
Copy link
Contributor Author

dhague commented Nov 26, 2015

Would somebody like to merge this? @geemus perhaps?

geemus added a commit that referenced this pull request Nov 30, 2015
[openstack] volume API v2 implementation
@geemus geemus merged commit 93bd728 into fog:master Nov 30, 2015
@geemus
Copy link
Member

geemus commented Nov 30, 2015

Thanks!

@dhague dhague deleted the openstack_volume_v2_api branch December 2, 2015 16:24
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.

5 participants