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.

Sign up for GitHub

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

Jump to bottom

fixes #14618 - Introduced configurable initialization of modules and providers #390

Closed
wants to merge 1 commit into from
Closed

fixes #14618 - Introduced configurable initialization of modules and providers #390

wants to merge 1 commit into from

Conversation

dmitri-d
Copy link
Member

@dmitri-d dmitri-d commented Mar 9, 2016

No description provided.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 9b822e1 must be in the format fixes #redmine_number - brief description.

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

@dmitri-d
Copy link
Member Author

dmitri-d commented Mar 9, 2016

Work in progress, although most functionality has been added.

This adds functionality to customize/override configuration and initialization of plugins and providers.

  • ability to update/add computed settings (as opposed to purely config-file based settings).
  • ability to add and execute custom validators
  • ability to start background services during plugin initialization

Configuration/initialization logic has been moved to https://github.com/theforeman/smart-proxy/pull/390/files#diff-11cebac8a6625c7bb985371a365a48e3R163 and https://github.com/theforeman/smart-proxy/pull/390/files#diff-11cebac8a6625c7bb985371a365a48e3R196.

Plugin dsl adds support for overriding the classes above, but also provides ability to individually override validator, dependency, dependency injection wirings loader classes.

Some questions that need answers:

  • I noticed that one of the common issues is referring to wrong plugin class when retrieving provider settings. I'd like to merge main plugin and provider settings (per provider).
  • Right now all runtime dependencies (think a bunch of requires that used to go in the after_activation block) are loaded before validators are being executed (to load custom validators classes). Instead, validator loader can perform that, which would allow us to delay loading of classes until after we know the configuration is valid.

Deep dive, examples, and docs are coming. I'll be happy to do a code walk through to help with the review(s).

@ekohl, @helge000, @GregSutcliffe, @domcleal: thoughts?

@ekohl
Copy link
Member

ekohl commented Mar 9, 2016

It would help me if I saw what would change for me as a plugin author because it's hard for me to derive the changes from this diff.

@dmitri-d
Copy link
Member Author

dmitri-d commented Mar 9, 2016

I will port #376 PR to highlight the changes.

@dmitri-d
Copy link
Member Author

[test]

@dmitri-d dmitri-d changed the title [DO NOT MERGE] Introduced configurable initialization of modules and providers Introduced configurable initialization of modules and providers Apr 13, 2016
@dmitri-d dmitri-d mentioned this pull request Apr 13, 2016
Closed
@dmitri-d
Copy link
Member Author

@ekohl: pls see #406 that demonstrates most of the changes in this PR.

@dmitri-d
Copy link
Member Author

I'm aware of build failures under 1.8.7 and am looking into them.

@domcleal domcleal changed the title Introduced configurable initialization of modules and providers ixes #14618 - Introduced configurable initialization of modules and providers Apr 14, 2016
@domcleal domcleal changed the title ixes #14618 - Introduced configurable initialization of modules and providers fixes #14618 - Introduced configurable initialization of modules and providers Apr 14, 2016
domcleal
domcleal reviewed Apr 22, 2016
View reviewed changes
lib/proxy/pluggable.rb
attr_reader :plugin_name, :version, :after_activation_blk, :class_loader

# Methods below define DSL for defining plugins
def after_activation(&blk)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method meant to be deprecated? There are references in the loading code that it's "legacy".

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at the moment, although it can be deprecated if I add support for blocks as means of loading classes, specifying di wiring, etc, all of which require class(es) atm.

@domcleal
Copy link
Contributor

  1. The changes to the public plugin API are difficult to understand, particularly the DI terminology and to understand how the new APIs are used. This would be a good time to improve the documentation, both to help me review and for future use.

The wiki page would be a good start, provided it has a warning to say that this is >= 1.12 only. I'm also quite concerned to understand which APIs (if any) are changing behaviour in this patch, are being deprecated, or are being removed.

@dmitri-d
Copy link
Member Author

The docs are in plans for the next week. I would suggest taking a look at #406 to see how plugins can use the new functionality (puppet_proxy_legacy module is probably one of the better examples).

@dmitri-d dmitri-d mentioned this pull request Apr 26, 2016
Closed
@dmitri-d
Copy link
Member Author

Done with the first pass at the docs: http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Smart-Proxy_Plugin

domcleal
domcleal reviewed May 9, 2016
View reviewed changes
lib/proxy/error.rb
@@ -1,5 +1,12 @@
module Proxy::Error
class BadRequest < StandardError; end
class Unauthorized < StandardError; end
class HttpError < StandardError
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related? I can't see where it's used.

@dmitri-d
Copy link
Member Author

dmitri-d commented May 10, 2016 •
edited
Loading

Rebased, fixed the issues pointed out in the review.

domcleal
domcleal reviewed May 10, 2016
View reviewed changes
test/dhcp_isc/server_isc_test.rb
@@ -40,7 +39,7 @@ def test_isc_provider_initialization
::Proxy::DhcpPlugin.load_test_settings(:server => 'a_server')
::Proxy::DHCP::ISC::Plugin.load_test_settings(:config => 'config_file', :leases => 'leases_file',
:omapi_port => '7777', :key_name => 'key_name',
:key_secret => 'key_secret')
:key_secret => 'key_secret', )
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the test on Ruby 1.8.7, but like the previous comment, I don't think this file needs any changes. Could you revert it to what's in develop please?

@dmitri-d
Fixes #14618 - refactored module initialization
2086b1b 
  to allow for:
    - support for multiple simultaneous providers
    - programmatically defined settings
    - custom validators
    - ability to customize module loading

  Also includes support for constructor-based dependency injection
@dmitri-d
Copy link
Member Author

fixed the two issues above.

@domcleal
Copy link
Contributor

Would you mind updating the wiki page again to reflect any recent changes (e.g. support for blocks)?

@domcleal
Copy link
Contributor

Merged as 342eeda, thanks @witlessbird.

@domcleal domcleal closed this May 11, 2016
@dmitri-d
Copy link
Member Author

Would you mind updating the wiki page again to reflect any recent changes (e.g. support for blocks)?

Done.

@dmitri-d dmitri-d deleted the custom-plugin-initializers branch May 24, 2016 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmitri-d @theforeman-bot @ekohl @domcleal @lzap