-
Notifications
You must be signed in to change notification settings - Fork 675
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
Reduce external gem dependencies: extract backup-fog into an internal plugin #1004
base: master
Are you sure you want to change the base?
Conversation
…emantics and related changes in rspec-mocks
Main gem specs are green. |
…change of constants to avoid collision with Fog module from dependency.
I think we're good! Of course there's some cleanup to do, but the main gem and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't review this on a technical level anymore, not without doing lots of read up on the source code. If someone else can do a more in depth review, please :)
Not sure if I would want to merge it as-is to the main master branch. We could make a new 6.0
branch for you to work on, and merge that into main when it's done.
gem.add_dependency "dropbox-sdk", "1.6.5" | ||
gem.add_dependency "flowdock", "0.4.0" | ||
# gem.add_dependency "fog", "~> 1.42" | ||
gem.add_dependency "hipchat", "1.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably also remove some gems like hipchat, because I don't think that service is online anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL that hipchat is no more 🕯️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove hipchat in a follow-up PR, for sake of clean separation.
6.0 branch sounds like a plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the few inline comments, I think we should discuss the namespacing/naming of things.
Shouldn't we do something like:
class Backup::PluginName::Storage::XXX
?
@@ -1,52 +0,0 @@ | |||
inherit_from: .rubocop_todo.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why dropping rubocop ? I don't remember if we discussed this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1003 here's why 😅
Moving plugins into a namespace on their own will enforce and underline the separation between "core" and plugins. I got a feeling from our previous conversations that we want to do the opposite, with keeping plugins in the same repo etc., so I haven't moved on with any new namespaces in code extracted to plugins. I also see it as redundant, since |
LGTM. Thanks ! |
Slightly different approach and therefore start from scratch after discussion in #1003
Mission: make backup modular for better maintainability. As shown, external gem dependencies can slow us down, including to a point where it's not possible to make
backup
compatible with Ruby 3+ as long as we have some of those dependencies.Approach: move
fog
using code (S3 and Rackspace Cloud Files) into an accompanying plugin gem that will be maintained separately from the mainbackup
gem.Progress:
rubocop
, at least for now. Newer ruby versions mean newer rubocop, and newer rubocop means many renames, new cops and basically hundreds of offenses by current codebase.backup
has green tests after extracting all fog-using code intobackup-fog
plugin and commenting-out its classes in autoloadbackup-fog
has green tests and loads whichever classes it needs from mainbackup
gem