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

Limit number of runtime dependencies for fog. #325

Closed

Conversation

driv3r
Copy link

@driv3r driv3r commented Apr 11, 2016

By default fog depends on all possible fog providers, which boosts unnecessary number of dependencies.

fog-core on the other hand, have minimum amount of deps, and mimimum number of classes, which leaves developer more space by using only required provider.

This commit reduces number of dependencies from 94 to 64.

By default fog depends on all possible fog providers,
which boosts unnecessary number of dependencies.

fog-core on the other hand, have minimum amount of deps,
and mimimum number of classes, which leaves developer
more space by using only required provider.

This commit reduces number of dependencies from 94 to 64.
@PikachuEXE
Copy link
Member

I am unable to process this since I don't enough knowledge about the internal structure
Please wait for @davidjrice

@driv3r
Copy link
Author

driv3r commented Apr 12, 2016

In general there are only two uses of fog class, and those are Fog::Storage to build initialize storage based on given configuration, and Fog::CDN. Both of those classes are within fog-core gem (Fog::Storage and Fog::CDN).

I've also added the fog-aws as development dependency for integration tests.

The only issue that I can see is the upgrade path, we should definitely mention in changelog that user is required to add fog-x to the Gemfile, where x stands for provider he's using.

In meantime I'll test it in personal project, and give feedback if there's anything to be still updated.

@driv3r
Copy link
Author

driv3r commented Apr 12, 2016

btw also thing to note is that there is similar PR #300 but it includes fog-aws instead of just fog-core

@borfd
Copy link

borfd commented Apr 12, 2016

👍

@driv3r
Copy link
Author

driv3r commented Apr 12, 2016

I've found another duplicate #295 with fog-aws instead of fog-core

@PikachuEXE
Copy link
Member

To reduce the no. of gem dependencies without breaking anything
It might be better to include all 3 gems for the 3 supported sources

  • aws
  • rackspace
  • google

@driv3r
Copy link
Author

driv3r commented Apr 13, 2016

sounds ok, but I would still leave it as option for user

@PikachuEXE
Copy link
Member

Either way I cannot judge what to do with this issue
I am just a helper (can't release stuff)

@jhass jhass mentioned this pull request Jun 17, 2016
5 tasks
@galori
Copy link

galori commented Jul 25, 2016

Does this intentionally not include fog/aws in runtime dependency to allow projects to include the one they want?

@galori
Copy link

galori commented Jul 25, 2016

Also FYI for anyone that wants to use this fork directly off of github - this fork is behind master several commits. I created my own fork of this fork that requires fog/aws and is updated with master, here:

master...galori:narrow-down-dependencies

@SuperTux88
Copy link

SuperTux88 commented Nov 24, 2016

@PikachuEXE What is the state of this PR? Has @davidjrice ever expressed his opinion about this topic? There are multiple PRs about it and I didn't find anything from him.

Some of these PRs are over a 1,5 years old, how long do you want to wait? Can we please find a solution for this? If @davidjrice doesn't want to decide it, someone else needs to make a decision.

@PikachuEXE
Copy link
Member

I agree.
I guess I will make a decision on whether to require fog-core or fog-aws soon.
I have been calling @davidjrice for several months (on Slack).
And since he rarely responds, I guess he is too busy to take care of this project.

Note that I am busy with my job too :S

@SuperTux88
Copy link

Thanks 👍

No worries, no need to rush, I only think we shouldn't wait another 1,5 years :)

@PikachuEXE
Copy link
Member

Just released 1.3.0 as "should be" last version of 1.x
Will start preparing 2.0.0 release now

Need to change README + Upgrade note
Might need a week or so (I need to steal time from my job to do this :P)

@PikachuEXE
Copy link
Member

@SuperTux88
PR #336 created, but doc might be incomplete
Please help me check :)

I will try to fix AWS integration text too before merging it

@SuperTux88
Copy link

I submitted a review to the PR, thanks for your work! 👍

@PikachuEXE
Copy link
Member

2.0.0 released
Have a try!

@PikachuEXE
Copy link
Member

This should be closed now I guess

@PikachuEXE PikachuEXE closed this May 19, 2017
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

5 participants