-
Notifications
You must be signed in to change notification settings - Fork 0
Implement boot-time plugin loading for improved performance and reliability #18
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
Conversation
Co-authored-by: GrantBirki <[email protected]>
Co-authored-by: GrantBirki <[email protected]>
Co-authored-by: GrantBirki <[email protected]>
@copilot please iterate on the changes you made and fully remove all dynamic plugin loading. Everything should be done at boot-time to ensure a consistent application during a deployment. It should be static in the sense that nothing gets dynamically loaded after the applications starts up. |
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.
Pull Request Overview
This PR implements boot-time loading of auth and handler plugins to improve request performance and reliability while preserving backward compatibility.
- Introduces a new
PluginLoader
that caches plugins at application startup. - Updates the core
Builder
and App modules to retrieve plugins from the in-memory registry, with dynamic fallback. - Expands and adjusts test setup to initialize and clean up the plugin registry.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
spec/unit/spec_helper.rb | Initialize PluginLoader for all tests |
spec/unit/lib/hooks/plugins/auth/hmac_spec.rb | Add require_relative to load shared spec helper |
spec/unit/lib/hooks/core/plugin_loader_spec.rb | New specs covering PluginLoader.load_all_plugins |
spec/unit/lib/hooks/core/config_loader_spec.rb | Ensure ENV cleanup after auth plugin dir test |
spec/unit/lib/hooks/app/helpers_spec.rb | Add require_relative to load shared spec helper |
spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb | Initialize and clear plugin registry in integration |
lib/hooks/core/plugin_loader.rb | New PluginLoader implementation for boot-time plugins |
lib/hooks/core/builder.rb | Invoke boot-time plugin loading during initialization |
lib/hooks/app/helpers.rb | Refactor helper to use cached plugins with fallback |
lib/hooks/app/auth/auth.rb | Refactor auth to use cached plugins with dynamic fallback |
.bundle/config | Update BUNDLE_PATH for CI environment |
Comments suppressed due to low confidence (3)
lib/hooks/core/plugin_loader.rb:59
- [nitpick] Handler lookup is case-sensitive while auth lookup downcases names. This inconsistency could confuse consumers. Consider normalizing handler keys (e.g.,
.downcase
) or documenting the exact matching behavior.
plugin_class = @handler_plugins[handler_name]
lib/hooks/app/helpers.rb:71
- The new
load_handler
logic—including cache hit, dynamic fallback, and error paths—needs dedicated unit tests to ensure correct behavior across all scenarios.
def load_handler(handler_class_name, handler_dir = nil)
lib/hooks/app/auth/auth.rb:34
- We should add tests for the new boot-time auth lookup and fallback dynamic loading paths (including error states) to maintain full coverage of
validate_auth!
.
auth_class = Core::PluginLoader.get_auth_plugin(auth_type)
.bundle/config
Outdated
@@ -1,6 +1,6 @@ | |||
--- | |||
BUNDLE_BIN: "bin" | |||
BUNDLE_PATH: "vendor/gems" | |||
BUNDLE_PATH: "/home/runner/work/hooks/hooks/vendor/bundle" |
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.
This file locked in an environment-specific absolute path. It’s best practice to remove .bundle/config
from version control or use a relative/common path to avoid CI/local mismatches.
Copilot uses AI. Check for mistakes.
…roach Co-authored-by: GrantBirki <[email protected]>
I've completely removed all dynamic plugin loading as requested. The application now uses a fully static approach where all plugins are loaded only at boot time via the PluginLoader. No fallback to dynamic loading exists anymore, ensuring consistent behavior during deployments. Key changes in commit cc2bfcb:
The application is now completely static after startup - no plugins are ever loaded dynamically during request processing. |
Overview
This PR addresses the issue where auth and handler plugins were being loaded dynamically during request processing, which is slow and can lead to unexpected failures. The solution implements boot-time plugin loading while maintaining full backward compatibility.
Changes Made
Core Implementation
PluginLoader
class (lib/hooks/core/plugin_loader.rb
) that loads all plugins at boot time into in-memory registriesBuilder
to call plugin loading during app initializationlib/hooks/app/auth/auth.rb
) to use cached plugins with fallback to dynamic loadinglib/hooks/app/helpers.rb
) to use cached plugins with fallback to dynamic loadingKey Features
Testing & Quality
spec/unit/lib/hooks/core/plugin_loader_spec.rb
with 85.37% total coverageconfig_loader_spec.rb
require_relative
inhmac_spec.rb
Example Usage
Performance Benefits
Backward Compatibility
Test Results
Fixes #17.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.