-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add Gem hook plugin support #748
Open
seanmil
wants to merge
1
commit into
sds:main
Choose a base branch
from
Secure-24:add_gem_hook_support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# frozen_string_literal: true | ||
|
||
module Overcommit::HookLoader | ||
# Responsible for loading custom hooks that users install via Gems | ||
class GemHookLoader < Base | ||
def load_hooks | ||
@config.enabled_gem_hooks(@context).map do |hook_name| | ||
underscored_hook_name = Overcommit::Utils.snake_case(hook_name) | ||
require "overcommit/hook/#{@context.hook_type_name}/#{underscored_hook_name}" | ||
create_hook(hook_name) | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
describe Overcommit::HookLoader::GemHookLoader do | ||
let(:hash) { {} } | ||
let(:config) { Overcommit::Configuration.new(hash) } | ||
let(:logger) { double('logger') } | ||
let(:context) { double('context') } | ||
let(:loader) { described_class.new(config, context, logger) } | ||
|
||
describe '#load_hooks' do | ||
subject(:load_hooks) { loader.send(:load_hooks) } | ||
|
||
before do | ||
context.stub(hook_class_name: 'PreCommit', | ||
hook_type_name: 'pre_commit') | ||
end | ||
|
||
it 'loads enabled gem hooks' do | ||
allow(config).to receive(:enabled_gem_hooks).with(context).and_return(['MyCustomHook']) | ||
|
||
allow(loader).to receive(:require). | ||
with('overcommit/hook/pre_commit/my_custom_hook'). | ||
and_return(true) | ||
allow(loader).to receive(:create_hook).with('MyCustomHook') | ||
expect(loader).to receive(:require).with('overcommit/hook/pre_commit/my_custom_hook') | ||
load_hooks | ||
end | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 may be misunderstanding, but since Overcommit hooks are themselves in the load path, won't this still return
true
even for built-in hooks?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.
Thank you for your thorough and thoughtful response.
My initial implementation was built within the core hooks loader, but there were drawbacks. It didn't provide any way for a user to indicate they didn't want to load the hooks, and then I later realized/remembered that when invoked outside of a bundler context the
$LOAD_PATH
doesn't have the gem path on it until it isrequire
d. Once I got done looking at trying to fix both of those problems it seemed to make more sense to put them in a separate loader. But I agree that folding them into an existing one might be useful.I decided that was okay because my thinking was that unlike hooks which are delivered from a repository, hooks from a Ruby gem have to be installed by the system/user. Either via
gem install
at the system level or by the user via abundle install
. Even using thegemfile
Overcommit configuration option won't just go to the Internet and fetch uninstalled gems - it tells you to usebundle install
. So, in both scenarios the user (or authorized system administrator) has had to be involved in intentionally putting the gems on the system. I didn't see the code exposure path here being any different than any other gem in the system.That said, if you strongly believe that gem-based hooks also need to be covered by the signature bit that probably wouldn't be hard to implement.
That's a great point. I debated quite a bit about putting the hooks in the same load path as the built-in Overcommit hooks. After reading your feedback I think that probably the best thing is to expect the hooks to be in some other load path and provide some way for an Overcommit hook add-on gem to provide that path to Overcommit. In terms of load precedence, I think the safest is that the built-in hooks always win.
Maybe supporting a configuration file somewhat like this makes sense:
It would be clear which were external/custom hooks, which gem provided them (if there were multiple) and there would be no chance of namespace conflicts.
Then in the custom hook Overcommit expects to be able to require
overcommit-custom/hooks
(the path provided in the config, plushooks
) and in there lives the normal hooks structure.I could see the above syntax being extendable to something like:
Where the path is resolved based on the gem path. I'm not sure I want to tackle inheritable configuration as part of this, just noting that I think it could be added.
Let me know what you think about the syntax above. I don't know if I'll have any more time to work on it in the near future, but I may.