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

Split drivers into individual classes #25

Merged
merged 23 commits into from
Jun 18, 2014
Merged

Conversation

thommay
Copy link

@thommay thommay commented Jun 11, 2014

By way of a point of discussion, I've broken out individual drivers into their own classes. This cleans up a lot of pretty epic switches, but I've mostly done it so I can eventually add ohai hints into the correct classes automatically.
This commit also adds a very tiny amount of rspec.

@jkeiser
Copy link
Contributor

jkeiser commented Jun 12, 2014

Will look at this really soon, I'm super crazy thrilled :)

@thommay
Copy link
Author

thommay commented Jun 12, 2014

I should've added a bit more rspec, this isn't correct right now. Rethinking a little...
(Problem is that I got the call chain for #compute_options_for the wrong way round.

Thom May added 4 commits June 13, 2014 08:48
@thommay
Copy link
Author

thommay commented Jun 13, 2014

@jkeiser I'm gonna stop now; this works for rackspace at least. Looking forward to your comments!


def self.new(driver_url, config)
provider = driver_url.split(':')[1]
require "chef_metal_fog/providers/#{provider.downcase}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to follow the pattern of the driver_init, we should probably follow the naming too, for predictability's sake--can you rename providers to provider_init?

This is interesting, as it allows someone to make a chef-metal-fog-blah driver. I like :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid duplication of the _init, it's probably worth adding a provider_class_for(provider) method.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re provider_init rather than providers - I thought about that, but I'm not sure it actually reflects what's going on; the driver_init stuff is just sticking the class name into the list of available classes, whereas the providers are actually doing the work too. Separating the initialization from the provider didn't seem valuable either, I think

Thom May added 5 commits June 16, 2014 18:49
@thommay
Copy link
Author

thommay commented Jun 16, 2014

@jkeiser I've made all the changes besides the provider_init one…

@jkeiser
Copy link
Contributor

jkeiser commented Jun 16, 2014

Thanks! I got interrupted halfway through, will find my way through the rest shortly. Your point about provider_init makes sense.

@jkeiser
Copy link
Contributor

jkeiser commented Jun 16, 2014

Other than the last comment, this looks really good :)

@thommay
Copy link
Author

thommay commented Jun 17, 2014

I think I like this approach a bit more :)

jkeiser added a commit that referenced this pull request Jun 18, 2014
Split drivers into individual classes
@jkeiser jkeiser merged commit cf0a641 into chef-boneyard:master Jun 18, 2014
viglesiasce pushed a commit to viglesiasce/chef-metal-fog that referenced this pull request Aug 19, 2014
Update README.md to show how to add per-machine provisioner options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants