Refactor with an eye toward separation of git-core and specific provider #20

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@wchrisjohnson
Contributor

wchrisjohnson commented Feb 25, 2014

I'm currently working on the rewrite of the OpenStack provider in a separate gem. As a result, the 'require' for the provider needs to occur within the context of the new gem, not fog-core.

@wchrisjohnson

This comment has been minimized.

Show comment
Hide comment
@wchrisjohnson

wchrisjohnson Feb 25, 2014

Contributor

Will this change break other providers, other than the Rackspace provider? It just occurred to me that it might be useful to validate that...

Contributor

wchrisjohnson commented Feb 25, 2014

Will this change break other providers, other than the Rackspace provider? It just occurred to me that it might be useful to validate that...

@tokengeek

This comment has been minimized.

Show comment
Hide comment
@tokengeek

tokengeek Feb 25, 2014

Member

Not sure about this.

That pattern got setup as a way to defer the require until the service/provider is first used.

So Fog::Identity.new(:provider => :openstack) should still work after require "fog" but would be the point `require "fog/openstack/identity" is required.

I may be missing something as to why it is being removed because it can be required multiple times if you must require it before the call to Identity.new.

Also not sure which other providers expose an identity service.

Member

tokengeek commented Feb 25, 2014

Not sure about this.

That pattern got setup as a way to defer the require until the service/provider is first used.

So Fog::Identity.new(:provider => :openstack) should still work after require "fog" but would be the point `require "fog/openstack/identity" is required.

I may be missing something as to why it is being removed because it can be required multiple times if you must require it before the call to Identity.new.

Also not sure which other providers expose an identity service.

@elight

This comment has been minimized.

Show comment
Hide comment
@elight

elight Feb 25, 2014

Best way I know to determine this is modify your fog dev env to use your local fog-core, run the test suite, and see what breaks. :D

On February 25, 2014 at 3:04:43 PM, Chris Johnson (notifications@github.com) wrote:

Will this change break other providers, other than the Rackspace provider? It just occurred to me that it might be useful to validate that...


Reply to this email directly or view it on GitHub.

elight commented Feb 25, 2014

Best way I know to determine this is modify your fog dev env to use your local fog-core, run the test suite, and see what breaks. :D

On February 25, 2014 at 3:04:43 PM, Chris Johnson (notifications@github.com) wrote:

Will this change break other providers, other than the Rackspace provider? It just occurred to me that it might be useful to validate that...


Reply to this email directly or view it on GitHub.

@wchrisjohnson

This comment has been minimized.

Show comment
Hide comment
@wchrisjohnson

wchrisjohnson Feb 25, 2014

Contributor

I have the new openstack provider (named openstackcommon) working locally with that line commented out. With that line of code in place, it fails.

We are replacing the current provider, not updating it. In the new gem, the folder structure is not the same. As I understand it, the fog//identity is used to refer to the file that contains the identity service provider. In the single fog project, all of the providers were under the fog project, and this require worked. In this new approach, that is no longer the case.

Does this help?

The more I think about this though, the more convinced I am that this might break existing providers if it goes in. Let me test this with the existing HP provider before moving forward here; we might have to adjust...

Contributor

wchrisjohnson commented Feb 25, 2014

I have the new openstack provider (named openstackcommon) working locally with that line commented out. With that line of code in place, it fails.

We are replacing the current provider, not updating it. In the new gem, the folder structure is not the same. As I understand it, the fog//identity is used to refer to the file that contains the identity service provider. In the single fog project, all of the providers were under the fog project, and this require worked. In this new approach, that is no longer the case.

Does this help?

The more I think about this though, the more convinced I am that this might break existing providers if it goes in. Let me test this with the existing HP provider before moving forward here; we might have to adjust...

@wchrisjohnson

This comment has been minimized.

Show comment
Hide comment
@wchrisjohnson

wchrisjohnson Feb 25, 2014

Contributor

@elight I ran the test suite locally - it works (see the Travis CI build report above too)

Contributor

wchrisjohnson commented Feb 25, 2014

@elight I ran the test suite locally - it works (see the Travis CI build report above too)

@elight

This comment has been minimized.

Show comment
Hide comment
@elight

elight Feb 25, 2014

Ah! I didn’t see the Travis report earlier. Guess it was still running?

On February 25, 2014 at 3:23:43 PM, Chris Johnson (notifications@github.com) wrote:

@elight I ran the test suite locally - it works (see the Travis CI build report above too)


Reply to this email directly or view it on GitHub.

elight commented Feb 25, 2014

Ah! I didn’t see the Travis report earlier. Guess it was still running?

On February 25, 2014 at 3:23:43 PM, Chris Johnson (notifications@github.com) wrote:

@elight I ran the test suite locally - it works (see the Travis CI build report above too)


Reply to this email directly or view it on GitHub.

@wchrisjohnson

This comment has been minimized.

Show comment
Hide comment
@wchrisjohnson

wchrisjohnson Feb 25, 2014

Contributor

Hey @elight how is your stuff set up to to be able to test/run stuff on top of fog-core? Is it? Or did you just depend on the test suite?

Trying to determine how to test the above change with the traditional HP provider...

Contributor

wchrisjohnson commented Feb 25, 2014

Hey @elight how is your stuff set up to to be able to test/run stuff on top of fog-core? Is it? Or did you just depend on the test suite?

Trying to determine how to test the above change with the traditional HP provider...

@wchrisjohnson

This comment has been minimized.

Show comment
Hide comment
@wchrisjohnson

wchrisjohnson Mar 26, 2014

Contributor

Close as Evan's PR #30 will correct this.

Contributor

wchrisjohnson commented Mar 26, 2014

Close as Evan's PR #30 will correct this.

@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Mar 31, 2014

Member

Thanks!

On Wed, Mar 26, 2014 at 4:01 PM, Chris Johnson notifications@github.comwrote:

Closed #20 #20.

Reply to this email directly or view it on GitHubhttps://github.com/fog/fog-core/pull/20
.

Member

geemus commented Mar 31, 2014

Thanks!

On Wed, Mar 26, 2014 at 4:01 PM, Chris Johnson notifications@github.comwrote:

Closed #20 #20.

Reply to this email directly or view it on GitHubhttps://github.com/fog/fog-core/pull/20
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment