-
Notifications
You must be signed in to change notification settings - Fork 985
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
fixes #16725 - add plugin extension point for smart proxies #3904
fixes #16725 - add plugin extension point for smart proxies #3904
Conversation
def smart_proxy_for(klass, name, options) | ||
@smart_proxies[klass.name] ||= {} | ||
@smart_proxies[klass.name][name] = options | ||
klass.register_smart_proxy(name, options) |
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 probably won't work well with class autoloading, as if klass
gets reloaded at any point the belongs_to association (etc) will be lost. You could either:
- Add an
included
block to the concern so after it's loaded, it queries Foreman::Plugin to reload any smart proxies on the model. - Remove the
@smart_proxies
stuff (though it's my preferred method) and useto_prepare
instead, which would be called on every development request (in the dev environment where reloading occurs).
|
||
child :dns => :dns do | ||
extends "api/v2/smart_proxies/base" | ||
Subnet.smart_proxies.keys.each do |proxy| |
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.
@object
?
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.
It has to be Subnet
, because @object
would be Subnet::Ipv4
for example. The smart proxies are registered only on `Subnet´.
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.
class_attribute
may help here, as its value is inherited by subclasses if a more specific subclass value isn't set. See domcleal@4d05ce2 for example.
Note that's specifically fetching, merging, then setting the value of smart_proxies_from_model rather than modifying the same hash, which should allow for subclasses to vary their registered smart proxies from the parent (they'll receive a copy of the parent + their own alterations).
@@ -1,4 +1,6 @@ | |||
module SubnetsHelper | |||
include SharedSmartProxiesHelper |
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.
Shouldn't be needed, all helpers get loaded usually. Ditto in others.
@@ -0,0 +1,3 @@ | |||
module RealmsHelper |
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.
Not required? All helpers should be loaded.
@@ -6,7 +6,7 @@ def initialize(args) | |||
|
|||
def validate_each(record, attribute, value) | |||
if value && !value.has_feature?(@options[:feature]) | |||
record.errors["#{attribute}_id"] << _(@options[:message]) | |||
record.errors["#{attribute}_id"] << _('does not have the %s feature') % @options[:feature] |
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.
Best to prefer @options[:message]
here if it's set, else (||
) use the built in one - it's more like Rails ones then, and it's possible a plugin is using this validator.
@@ -3,3 +3,9 @@ object @domain | |||
extends "api/v2/domains/base" | |||
|
|||
attributes :fullname, :dns_id, :created_at, :updated_at | |||
|
|||
@object.smart_proxies.keys.each do |proxy| |
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'd suggest putting this into a partial like api/v2/taxonomies/children_nodes, it's identical in every location.
@@ -73,7 +73,10 @@ | |||
|
|||
<%= host_puppet_environment_field f %> | |||
|
|||
<%= puppet_master_fields f, true, @host.hostgroup && @host.hostgroup_id_was.nil? %> | |||
<%= smart_proxy_fields f, | |||
:can_override => true, |
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.
Nitpick: indentation should be more than the smart_proxy_fields
word itself.
f63d202
to
bf610a4
Compare
@domcleal : Updated. Thanks for your comments and domcleal/foreman@4d05ce2! I have also added api doc handling. It has some redundancy but it am unsure how to write an extension point for this without having to monkey-patch apipie-rails. So I haven't really looked into that. |
@@ -1,3 +1,5 @@ | |||
object @realm | |||
extends "api/v2/realms/base" | |||
extends "api/v2/smart_proxies/children_nodes" | |||
|
|||
attributes :name, :realm_proxy_id, :realm_type, :created_at, :updated_at |
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.
realm_proxy_id
and _name
should be replaced by the smart_proxy/children_nodes too I think, it can be generated easily.
Ditto in other views, e.g. host has puppet_proxy_id/puppet_ca_proxy_name which can be replaced.
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.
@domcleal : I saw the params, however we break compatibility if we just remove these. What is the normal procedure for this?
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.
Oh sorry - I hadn't meant to remove them. I meant to replace the explicit listings of attributes :realm_proxy_id
by code that automatically calls attributes
with _id
and _name
for each registered smart proxy association. IDs and names for new associations from plugins will automatically appear in the output too then, not just through full child nodes.
bf610a4
to
8e0bb69
Compare
@domcleal : Added the requested change and parameter_filter handling. Hope, we have all covered now. The smart proxy params for the host model were defined in |
I just tested this a little more. Somehow the inheritance thing still does not work. I need to take a closer look. |
8e0bb69
to
01185e9
Compare
@domcleal : I reworked the registration from plugins a bit. They are also inherited now. |
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.
Thanks, other than one small inline comment the registration system does seem to be more reliable now and it works well.
It would be reassuring if you could add some tests (either to the concern tests or subnet model tests) to check that the inheritance is indeed working between Subnet, Subnet::Ipv4/6 as I can't see any coverage of this right now.
else | ||
message = _(@options[:message]) | ||
end | ||
record.errors["#{attribute}_id"] << message |
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.
Use record.errors.add("#{attribute}_id", message)
, it's the preferred API. (Deprecated 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.
Done.
No, probably a mistake from when protected attributes were initially added.
Per the review comment, it'd be great if there was some test coverage of this, I don't see it. |
799eb83
to
1843c17
Compare
The tests run fine on my laptop. I have rebased the code. Let's see if they run through now. |
It's an interference between some of the unit tests that permanently change the behaviour of models via the plugin interface, then the controller tests fail later. You can reproduce it with:
You may want to stub some of the class modifications (e.g. adding validators) in the plugin unit tests rather than letting them modify the class (and attempting to revert changes). |
1843c17
to
acc6b80
Compare
Thanks a lot, @domcleal . You are awesome. Saved me the time of looking into this. That is why the test now runs against the "Awesome" module defined in the test class. Well, and it should fix the tests. I also renamed |
acc6b80
to
0e1a508
Compare
There is still one failing test. I'll investigate tomorrow. |
0e1a508
to
9560791
Compare
@domcleal : That was it. Tests are finally green. |
Merged, thanks @timogoebel! Please could you add a section to http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin, noting that it requires Foreman 1.14 or higher. |
I added the section to the docs. |
Nice patch! |
This commit adds the ability to easily add smart proxies to models from plugins. This also includes tiny dsl to specify the proxies in a model directly.
The proxies automatically show up in the model's form and api.
If this gets merged, I'll send a pr to foreman_discovery to make use of the feature.