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

[core] Deprecate Fog::Connection #2711

Merged
merged 3 commits into from
Feb 27, 2014
Merged

Conversation

tokengeek
Copy link
Member

This reapplies the deprecation warnings that were originally part of
7ee3535 when using Fog::Connection request.

Unfortunately the Mock world does not exercise any of the code affected
by this change so I missed that I had adjusted the signature of the
method.

If you pass a parser argument into Fog::Connection you should switch to
using Fog::XML::SAXParserConnection and pass the parser as the first
argument to #request

If you don't it is better to use Fog::Core::Connection to get rid of the
backwards compatibility code and XML dependency.

This will make fog VERY NOISY since you'll get output for every request
on out of date services.

@tokengeek
Copy link
Member Author

Okay - after #2710 it's obvious that I can't rely on the mock tests to be remotely accurate for lower level changes since I missed a blinding obvious error of using the wrong number of arguments.

So even if I had the time to update all the XML providers, I won't be able to test them.

I'm now also wondering if the SAXParserConnection will even work since it requires a parser each time.

If any providers can test this PR and replace their references to Fog::Connection (and your dependency on the XML stuff if you only use JSON) it'll save me a lot of time.

If the XML stuff doesn't work I'll push the contents of Fog::Connection down to Fog::XML::Connection

@tokengeek
Copy link
Member Author

The fail looks like another oddity, unrelated against Jruby.

There are a number of deprecations for this in the mock test run on Travis.

This implies that real connections (or at least Fog::Connection#request) is being used in some places. This may be undesired if real API calls are being made to real services.

This reapplies the deprecation warnings that were originally part of
7ee3535 when using Fog::Connection request.

Unfortunately the Mock world does not exercise any of the code affected
by this change so I missed that I had adjusted the signature of the
method.

If you pass a parser argument into Fog::Connection you should switch to
using Fog::XML::SAXParserConnection and pass the parser as the first
argument to `#request`

If you don't it is better to use Fog::Core::Connection to get rid of the
backwards compatibility code and XML dependency.

This will make fog VERY NOISY since you'll get output for every request
on out of date services.
Attempting to move to using Fog::XML::SAXParserConnection directly
failed because the arguments changed.

This adds another compatibility level with two key differences:

1) It's namespaced within XML so stands out as not being part of core
2) It is NOT creating deprecation warnings so can be used as the final
   step if rewriting to use SAXParserConnection is undesired

So when merged Fog::Connection usage will create noise.

Fog::XML::Connection works the same way and will be extracted to
"fog/xml" when we get to it.

Fog::Core::Connection just wraps Excon.request and leaves the response
body parsing to the provider.
Unlike last attempt this replaces Fog::Connection with
Fog::XML::Connection which should be directly compatible.

Fog::Connection is there for old PRs but should be removed real soon.

Providers using JSON should be able to replace "XML" with "Core" within
their code to cut down on the dependency.

If I get the time I may attempt to clean up some but testing with Mock
will mean that is mostly educated guesswork.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0e1daf3 on connection_deprecation_warning into 45f9826 on master.

@geemus
Copy link
Member

geemus commented Feb 27, 2014

@tokengeek - yeah, not sure how best to raise confidence in this. Let me know if I can help though.

tokengeek added a commit that referenced this pull request Feb 27, 2014
@tokengeek tokengeek merged commit 7a152a2 into master Feb 27, 2014
@tokengeek tokengeek deleted the connection_deprecation_warning branch February 27, 2014 22:24
@geemus
Copy link
Member

geemus commented Feb 28, 2014

Thanks!

On Thu, Feb 27, 2014 at 4:24 PM, Paul Thornthwaite <notifications@github.com

wrote:

Merged #2711 #2711.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2711
.

@elight
Copy link

elight commented Apr 8, 2014

So I may have just run into a problem due to this. With fog 1.21.0, I'm seeing...

This appears to be because fog/xml/connection wasn't required anywhere. Adding a require in fog/rackspace/core for this, I also needed fog/xml/sax_parser_connection. I required that in xml/connection. That seems to resolve it.

I hope I'm wrong but this seems to be panning out. This is not good.

@tokengeek
Copy link
Member Author

@elight - Sorry, I did as much as the tests would allow.

If you needed XML then require "fog/xml" should have resolved it but working on that and the gem has not been a priority.

If you didn't need XML then Fog::Core::Connection was probably what I should have added in.

I may have just been when I was doing a mass update I used the wrong connection without the require.

Unfortunately with incomplete tests and dozens of providers it was inevitable.

@tokengeek
Copy link
Member Author

Also I think it's something that only turns up if you are requiring just fog/rackspace (based on skeleton.rb) since requiring fog should already pull in fog/xml

So unless we create Travis jobs that independently require one provider at a time it's not something that will jump out without a bug report.

Create a new issue and depending on which of the two options you need let's see if we can get @geemus to do a new release ASAP.

@elight
Copy link

elight commented Apr 8, 2014

Ah... ok. Yes, exactly, I am only requiring 'fog/rackspace'. However, this is likely to become the norm moving forward as fog has a large footprint when requiring all providers. There's already an open ticket on carrierwave (before I opened a duplicate myself by accident) to use the require provider feature.

@elight
Copy link

elight commented Apr 8, 2014

Damn. Either @krames or I need to be triaging all of these PRs on a regular basis as well...

This seems to point increasingly toward formalizing a "Fog Core" team (not to be confused with fog-core) as I've alluded to in the Summit questions. Doubly necessary if we're going to continue pushing toward making big changes leading up to a 2.0 release.

But then I digress into discussing the Summit. I'll stop here. ;-) ;-)

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.

None yet

4 participants