rubygems icon indicating copy to clipboard operation
rubygems copied to clipboard

add plugin hooks for bundle check

Open nijikon opened this issue 5 years ago • 6 comments

Description:

I want to be able to execute a plugin when bundle check is run.

What was the end-user or developer problem that led to this PR?

I'm not able to execute a plugin when bundle check is run.

What is your fix for the problem, implemented in this PR?

Introduce before-check-all and after-check-all hooks when bundle check is run.


Tasks:

  • [x] Describe the problem / feature
  • [ ] Write tests
  • [x] Write code to solve the problem
  • [ ] Get code review from coworkers / friends

I will abide by the code of conduct.

nijikon avatar Jul 14 '20 19:07 nijikon

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

welcome[bot] avatar Jul 14 '20 19:07 welcome[bot]

Hi @nijikon! :wave:

Could you explain your use case? As I understand it, plugin hooks are to allow hooking into internal events, like bundler installing or requiring a gem, not for hooking into the execution of specific CLI commands. If we were to add something like this, wouldn't we need to add plugin hooks to signal the execution of every single CLI command? That doesn't sound right to me. Maybe your use case is valid, but there's a better place for the hooks you want to add that's not inside the implementation of a particular CLI command. Or maybe you need to do whatever you need to do, and then call bundle check, or viceversa, and there's no need to change bundler at all.

deivid-rodriguez avatar Jul 15 '20 12:07 deivid-rodriguez

I'm sorry that I didn't describe my use case at all. Let me fix that.

Let's say that you have a project in which you don't update gems. When you run your CI build, you are using bundle check || bundle install to check if all the dependencies are satisfied, so you don't have to run bundle install.

What we do in our diffend-ruby plugin, we use before-install-all hook, to be able to protect you so you won't install a malicious gem to your local/staging/production environment.

The problem starts when you don't update your gems and use bundle check command. There is no way to hook up to the runtime to run the security checks. That's why I wanted to introduce those hooks. I already tested it, and it works just didn't write specs, hence the draft PR.

nijikon avatar Jul 16 '20 15:07 nijikon

@nijikon reading into your use-case, i get the feeling that adding a plugin command for your gem might be the better solution. ie: bundler check-security or something alike.

I agree with the points @deivid-rodriguez raised, i don't think adding hooks into CLI commands is something we want to support. But reading your use-case made me think lots about an alternative solution. What if we add a feature to allow plugins to add custom checks to bundler check? It would work the same as plugin commands. When the user runs bundler check, it will run the default set of bundler checks, but then a plugin could register extra checks.

colby-swandale avatar Jul 17 '20 12:07 colby-swandale

Adding a plugin command is a bit too much, in my opinion. It would require the user to do an extra step, modify their CI, capistrano deployment etc. I don't want that. I want them to add the plugin and without any additional action.

To be honest the only reason why I put it in CLI::Check is because a lack of proper wrapper to handle the check. If you look into how it is implemented in the CLI::Install, it executes the Installer here, https://github.com/rubygems/rubygems/blob/master/bundler/lib/bundler/cli/install.rb#L64. Which is a wrapper for plugin hooks in my opinion. https://github.com/rubygems/rubygems/blob/master/bundler/lib/bundler/installer.rb#L22-L28. We could easily implement it in CLI::Install like this without the wrapper

Plugin.hook(Plugin::Events::GEM_BEFORE_INSTALL_ALL, definition.dependencies)
Bundler::Installer.new(Bundler.root, definition).run(options)
Plugin.hook(Plugin::Events::GEM_AFTER_INSTALL_ALL, definition.dependencies)

I'm happy to jump on a call and brainstorm this.

nijikon avatar Jul 17 '20 13:07 nijikon

What we do in our diffend-ruby plugin, we use before-install-all hook, to be able to protect you so you won't install a malicious gem to your local/staging/production environment.

That links goes no where for me. Maybe you still need to publish the sources or the link is wrong?

@nijikon reading into your use-case, i get the feeling that adding a plugin command for your gem might be the better solution. ie: bundler check-security or something alike.

I was thinking exactly the same thing as @colby-swandale, that seems to be calling for a custom bundle command to me. In fact, it seems quite similar to bundle audit? But I guess you have a point that it is an extra steps for your users.

Something I didn't quite get from your explanation is in which case these new hooks would be useful and the install hooks won't do the trick. Like do some of your users run bundle check but don't try to install gems at all?

deivid-rodriguez avatar Jul 17 '20 13:07 deivid-rodriguez