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

Reduce external gem dependencies: extract backup-fog into an internal plugin #1004

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tomash
Copy link
Contributor

@tomash tomash commented Feb 26, 2024

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 main backup gem.

Progress:

  • First blood: removed fog dependencies into a separate gem. This already gives us Ruby 3.2(.2) compatibility.
  • Removed 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 into backup-fog plugin and commenting-out its classes in autoload
  • backup-fog has green tests and loads whichever classes it needs from main backup gem

@tomash
Copy link
Contributor Author

tomash commented Feb 27, 2024

Main gem specs are green.
backup-fog have quite a few failures and it seems I should either stick to older rspec-mocks version or fix them the hard way: rspec/rspec-mocks#1460

@tomash tomash changed the title [WIP] Reduce external gem dependencies Reduce external gem dependencies: extract backup-fog into an internal plugin Feb 27, 2024
@tomash tomash marked this pull request as ready for review February 27, 2024 13:36
@tomash
Copy link
Contributor Author

tomash commented Feb 27, 2024

I think we're good! Of course there's some cleanup to do, but the main gem and backup-fog plugin both have green tests now.

Copy link
Member

@tombruijn tombruijn left a 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.

plugins/backup-fog/.rspec_status Outdated Show resolved Hide resolved
lib/backup/logger.rb Outdated Show resolved Hide resolved
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"
Copy link
Member

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.

Copy link
Contributor Author

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 🕯️

Copy link
Contributor Author

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.

@tomash
Copy link
Contributor Author

tomash commented Mar 8, 2024

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.

6.0 branch sounds like a plan

Copy link
Contributor

@elthariel elthariel left a 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
Copy link
Contributor

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1003 here's why 😅

lib/backup.rb Outdated Show resolved Hide resolved
plugins/backup-fog/CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved
plugins/backup-fog/LICENSE.txt Outdated Show resolved Hide resolved
@tomash
Copy link
Contributor Author

tomash commented Mar 12, 2024

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 ?

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 class Backup::Storage::XXX will be provided only by plugin anyway, so there's no need to repeat plugin name in the module hierarchy.

@elthariel
Copy link
Contributor

LGTM. Thanks !

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

3 participants