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

Provider based case statements in services #16

Closed
tokengeek opened this issue Feb 14, 2014 · 12 comments
Closed

Provider based case statements in services #16

tokengeek opened this issue Feb 14, 2014 · 12 comments
Assignees
Labels
Milestone

Comments

@tokengeek
Copy link
Member

The factories for services appear to have the same problem based on a case statement around some providers.

So when Fog::Compute[:provider] or Fog::Compute.new(:provider => "provider") is called in almost all cases we require fog/provider/service then return a new instance of Fog::Provider::Service

I'm pretty sure that we can scrap most of this stuff.

  • Some providers are just there but have no special behaviour.
  • new_servers is just a wrapper around bare_metal_cloud and can probably go.
  • Rackspace and HP use the factory to switch on version. I think this can probably be pushed down to a factory for each provider. Fog::Compute[:hp] => Fog::HP::Compute.new(:version => "v1") => Fog::HP::ComputeV1.new
  • Anyone missing from Fog.providers can be added.
  • Might be some cases where symbol and class name are awkward to constantize vclouddirector => VcloudDirector

The behaviour is so similar / copy pasted that I think we can eliminate most of this.

We could even add something and enumerate over all providers asking if they recognise the symbol and use that.

Fog::Brightbox.recognise_provider?(:new_servers) # => false
Fog::BareMetalCloud.recognise_provider?(:new_servers) # => true
Fog::VcloudDirector.recognise_provider?(:vclouddirector) # => true

module Fog
  def self.new(options)
    provider_key = get_provider_from(options)
    provider_server = Fog.providers.detect { |prov| prov.recognise_provider?(provider_key)
   require "fog/{provider_service.name}/service"
   Fog::{Provider}.new(remaining_options...)
  end
end

Basically we don't want anything in core that doesn't need to be there or needs updating when a new provider is added.

Examples:

https://github.com/fog/fog-core/blob/master/lib/fog/compute.rb#L13-L62
https://github.com/fog/fog-core/blob/master/lib/fog/storage.rb#L10-L27

@elight
Copy link

elight commented Feb 14, 2014

Looked to he examples.  Concur. That code must get pushed down to the providers.

On Fri, Feb 14, 2014 at 6:40 AM, Paul Thornthwaite
[email protected] wrote:

The factories for services appear to have the same problem based on a case statement around some providers.
So when Fog::Compute[:provider] or Fog::Compute.new(:provider => "provider") is called in almost all cases we require fog/provider/service then return a new instance of Fog::Provider::Service
I'm pretty sure that we can scrap most of this stuff.

  • Some providers are just there but have no special behaviour.
  • new_servers is just a wrapper around bare_metal_cloud and can probably go.
  • Rackspace and HP use the factory to switch on version. I think this can probably be pushed down to a factory for each provider. Fog::Compute[:hp] => Fog::HP::Compute.new(:version => "v1") => Fog::HP::Compute.new
  • Anyone missing from Fog.providers can be added.
  • Might be some cases where symbol and class name are awkward to constantize vclouddirector => VcloudDirector
    The behaviour is so similar / copy pasted that I think we can eliminate most of this.
    We could even add something and enumerate over all providers asking if they recognise the symbol and use that.
Fog::Brightbox.recognise_provider?(:new_servers) # => false
Fog::BareMetalCloud.recognise_provider?(:new_servers) # => true
Fog::VcloudDirector.recognise_provider?(:vclouddirector) # => true
module Fog
  def self.new(options)
    provider_key = get_provider_from(options)
    provider_server = Fog.providers.detect { |prov| prov.recognise_provider?(provider_key)
   require "fog/{provider_service.name}/service"
   Fog::{Provider}.new(remaining_options...)
  end
end

Basically we don't want anything in core that doesn't need to be there or needs updating when a new provider is added.
Examples:
https://github.com/fog/fog-core/blob/master/lib/fog/compute.rb#L13-L62

https://github.com/fog/fog-core/blob/master/lib/fog/storage.rb#L10-L27

Reply to this email directly or view it on GitHub:
#16

@tokengeek
Copy link
Member Author

My examples got a bit muddled (bad namespaces) but I hope the thinking is still sound.

Existing behaviour...

Fog::Compute.new(provider: :rackspace, version: :v1) # => Fog::Compute::Rackspace.new
Fog::Compute.new(provider: :rackspace, version: :v2) # => Fog::Compute::RackspaceV2.new

One possible implementation where :version is passed to the assumed class of Fog::Compute::{provider}

Fog::Compute::Rackspace.new(option: true) # => Fog::Compute::Rackspace.new(option: true)
Fog::Compute::Rackspace.new(version: :v1, option: true) # => Fog::Compute::Rackspace.new(option: true)
Fog::Compute::Rackspace.new(version: :v2, option: true) # => Fog::Compute::RackspaceV2.new(option: true)

Unfortunately that's making the initializer a second level of "factory" accounting for the version.

Maybe we should consider making version a part of the service factory thing.

@mwhagedorn
Copy link
Contributor

its funny we have a developer book club at HP cloud and are looking at some of the same issues, in a general sense. Nice to see a real application of it

@elight
Copy link

elight commented Feb 14, 2014

Of course, we can’t yet use JSON-style hashes though because 1.8.7. Not just yet. ;-)

@wchrisjohnson
Copy link
Contributor

I'm THRILLED that that code jumped out at you guys as a code smell - it sure did to me when I first saw it.

If we operate based on the idea that we are all-in on openstack, and we want to focus as much of our effort on the openstack provider (push as much code as possible there), the following emerges:

fog-core (gem) <-- fog-openstack (gem) <-- fog-openstack- (gem)

Mike and I are going to tackle authentication as the first order of business, and hopefully this will allow us to flesh out the relationships between the above three gems and bring to light any muddiness in our thinking ;-)

@tokengeek
Copy link
Member Author

@elight Of course. I found using hash rockets taking too much space for the examples which in my mind were barely more than pseudocode!

Not sure if I like the idea that Fog::Compute::Provider.new could return a V2 instead but if the assumption that Fog::Compute[:provider] will require and return an instance of Fog::Compute::Provider then the namespace is already in use for both Rackspace and HP.

Feels messy overloading the initializer.

@mwhagedorn
Copy link
Contributor

@elight whats the drill on that? How do we handle deprecation here? I will add a lot more questions, more relevant to openstack on the TNG project on this, for instance when does Keystone v1 die? (or Nova v1 for that matter)

@elight
Copy link

elight commented Feb 14, 2014

@mwhagedorn Is there a need to deprecate something here? If the behavior gets extracted from fog-core and pushed down into the HP, Rackspace, et al providers then the behavior remains the same while the implementation changes. And that's a good thing. 😄

Can't speak to when Keystone V1 or Nova V1 dies. As I mentioned Wed, I haven't heard anything about Keystone V1 going away anytime even though few people use it with Rackspace AFAIK. I know nada about Nova V1 WRT Rackspace right now.

@wchrisjohnson
Copy link
Contributor

Makes sense @elight - that's definitely specific to a given provider.

@mwhagedorn
Copy link
Contributor

@elight .. yeah just thinking too much about the specifics, and we arent there yet. abstract! abstract!

@geemus
Copy link
Member

geemus commented Feb 17, 2014

Seems good 👍

@stale
Copy link

stale bot commented Jul 30, 2018

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 30, 2018
@stale stale bot closed this as completed Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants