fog / fog-google

Fog for Google Cloud Platform
MIT License
98 stars 146 forks source link

test fail on fog/fog from google monitoring init? #123

Closed geemus closed 8 years ago

geemus commented 8 years ago

https://travis-ci.org/fog/fog/jobs/125586258#L543

I tried to figure out whats up, as it seems to work for other stuff, but not sure why this isn't working. Any help digging in would be appreciated. Thanks!

selmanj commented 8 years ago

I looked into this a bit, even though I'm fairly new to this project.

If I try to load the monitoring client inside bundle console I get a warning not visible in the travis output:

jsselman@dev:~/fog (master u= origin/master)🍕  bundle console
Resolving dependencies...
irb(main):001:0> Fog::Monitoring["Google"]
[fog][WARNING] Please install the google-api-client gem before using this provider
Fog::Service::NotFound: google has no monitoring service
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/fog-core-1.38.0/lib/fog/core/services_mixin.rb:18:in `rescue in new'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/fog-core-1.38.0/lib/fog/core/services_mixin.rb:8:in `new'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/fog-core-1.38.0/lib/fog/monitoring.rb:12:in `new'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/fog-core-1.38.0/lib/fog/core/services_mixin.rb:4:in `[]'
        from (irb):1
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/cli/console.rb:14:in `run'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/cli.rb:319:in `console'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/vendor/thor/lib/thor.rb:359:in `dispatch'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/vendor/thor/lib/thor/base.rb:440:in `start'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/cli.rb:10:in `start'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/exe/bundle:19:in `block in <top (required)>'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/lib/bundler/friendly_errors.rb:7:in `with_friendly_errors'
        from /home/jsselman/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/bundler-1.11.2/exe/bundle:17:in `<top (required)>'
        from /home/jsselman/.rbenv/versions/2.2.2/bin/bundle:23:in `load'
        from /home/jsselman/.rbenv/versions/2.2.2/bin/bundle:23:in `<main>'irb(main):002:0>

(Note the warning to install google-api-client before using the provider).

If I add the dependency, the error no longer occurs.

Temikus commented 8 years ago

It seems that the problem is in fog-core: /lib/fog/core/services_mixin.rb

    def service_provider_constant(service_name, provider_name)
      Fog.const_get(provider_name).const_get(service_name)
    rescue NameError  # Try to find the constant from in an alternate location
      Fog.const_get(service_name).const_get(provider_name)
    end

Whatever you enter, it never actually returns a name error as long as the provider or service exist somewhere within Fog. For example I can ask for a service Google in Google namespace or Rackspace in Google namespace 😕 :

[21] pry(Fog::Monitoring)> Fog.const_get("Google").const_get("Google")
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
=> Google
[22] pry(Fog::Monitoring)> Fog.const_get("Google").const_get("Rackspace")
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
/Users/temikus/.rbenv/versions/2.1.8/lib/ruby/gems/2.1.0/gems/pry-0.10.3/lib/pry/pager.rb:186: warning: instance variable @pager not initialized
=> Rackspace

I'll continue digging, but if anyone has an idea of how to fix this quickly - do tell.

geemus commented 8 years ago

@Temikus maybe we should just modify that test to have an exception for google in particular (and just skip it) since it errors in this unexpected way? It's meant to be a really high level test anyway and I'm not sure it is all that useful in the first place.

Temikus commented 8 years ago

@geemus Can it be an indicator of loading issues though? If you don't think so - I can quickly make a corner-case or delete those specs.

Temikus commented 8 years ago

/CC @tokengeek since the tests in question were added by him. Paul, can you chime in on whether those are still required or not?

tokengeek commented 8 years ago

It was to support the transition between Fog::Compute::Google to Fog::Google::Compute which we had wanted to move providers to their own namespaces.

I've not kept up with the ongoing changes but looking at the failing test it looks like it is expecting an ArgumentError but is just getting a new Fog::Service::NotFound.

So I'm guessing the API has changed.

So either all providers should raise NotFound if they don't exist or Google should return ArgumentError

So I think it's still useful for consistency testing of the API otherwise providers start behaving differently.

geemus commented 8 years ago

@Temikus could we tweak the order of things in that provider so the credentials-missing error comes before the require error? I think maybe that would fix the test as well.

Temikus commented 8 years ago

@geemus @tokengeek To be clear - the current problem is in the error that is raised in fog-core itself (which is a NameError), not Fog-google, due to the Namespace issue outlined above. So even if I do move the load of the gem further down the code (which I tried), the test will still fail. Same goes if I remove LoadError from the rescue block:

    rescue LoadError, NameError  # Only rescue errors in finding the libraries, allow connection errors through to the caller
      raise Fog::Service::NotFound, "#{provider} has no #{service_name.downcase} service"
    end

To illustrate:

From: /Users/temikus/Code/fog-devel/fog-core/lib/fog/core/services_mixin.rb @ line 38 Fog::ServicesMixin#service_provider_constant:

    37: def service_provider_constant(service_name, provider_name)
 => 38:   Fog.const_get(service_name).const_get(provider_name)
    39: rescue NameError  # Try to find the constant from in an alternate location
    40:   Fog.const_get(provider_name).const_get(service_name)
    41: end

[1] pry(Fog::Monitoring)> Fog.const_get(service_name).const_get(provider_name)
=> Google
[2] pry(Fog::Monitoring)> service_name
=> "Monitoring"
[3] pry(Fog::Monitoring)> Fog.const_get(provider_name).const_get(service_name)
=> Fog::Google::Monitoring

I'll keep digging meanwhile, but this doesn't seem as trivial as it sounds :/

Temikus commented 8 years ago

Ok, I think I got it. By default const_get searches ancestors and Object. It looks like in our case we have Google in Monitoring ancestors' Module namespace, however, it returns only the top-level namespace by definition, which later screws up the logic (see prev. comment):

[2] pry(Fog::Monitoring)> ls Fog
constants:
  Account       Baremetal       CDN         Core            Dreamhost   Glesys    InternetArchive  Logger      NFV            Ovirt            Rage4        ServicesMixin  Terremark       Vmfusion       Xml
  Aliyun        BareMetalCloud  Clodo       CurrentMachine  Dynect      GoGrid    Introspection    Metering    Ninefold       PagedCollection  Riakcs       Softlayer      Time            Volume         XML
  Association   Billing         CloudSigma  Deprecation     Ecloud      Google    Joyent           Mock        OpenNebula     Parsers          RiakCS       SSH            ToHashDocument  Voxel          Zerigo
  Associations  Bin             Cloudstack  DigitalOcean    Errors      HMAC      Json             Model       OpenStack      PowerDNS         SakuraCloud  Storage        UUID            VPN
  Atmos         BinSpec         Collection  DNS             Fogdocker   IBM       JSON             Models      Openstack      ProfitBricks     SCP          StormOnDemand  Vcloud          Vsphere
  Attributes    Bluebox         Compute     DNSimple        Formatador  Identity  Linode           Monitoring  Openvz         Provider         Serverlove   StringifyKeys  VcloudDirector  WhitelistKeys
  AWS           Brightbox       Connection  DNSMadeEasy     Generators  Image     Local            Network     Orchestration  Rackspace        Service      Support        VERSION         XenServer

I submitted a PR but please take a look to confirm.

geemus commented 8 years ago

Good catch, thanks!