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.

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 #14036,#16722 - use plugin hook for subnet smart proxy #303

Merged
merged 1 commit into from Oct 12, 2016
Merged

fixes #14036,#16722 - use plugin hook for subnet smart proxy #303

merged 1 commit into from Oct 12, 2016

Conversation

timogoebel
Copy link
Member

@lzap
Copy link
Member

lzap commented Sep 30, 2016

Thanks, was busy today, will take a look and the core patch as well! Huge improvement, was driving me crazy :-)

@timogoebel
Copy link
Member Author

@lzap : Just as a fair warning: Expect more PRs here. At work discovery is now part of our production installation. Looks great, so far. Thanks.

@timogoebel
Copy link
Member Author

Please do not merge, yet. I still need to add the "api_description" to this.

@timogoebel
Copy link
Member Author

Added the missing api description. Ready for review & testing. 🚢

@lzap lzap self-assigned this Oct 11, 2016
lzap
lzap reviewed Oct 12, 2016
View reviewed changes
foreman_discovery.gemspec
@@ -21,6 +21,4 @@ Gem::Specification.new do |s|
s.homepage = %q{http://github.com/theforeman/foreman_discovery}
s.licenses = ["GPL-3.0"]
s.summary = %q{MaaS Discovery Plugin for Foreman}

s.add_dependency 'deface', '< 2.0'
Copy link
Member

Choose a reason for hiding this comment

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

👏

lzap
lzap approved these changes Oct 12, 2016
View reviewed changes
Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Great improvement, thanks.

@lzap
Copy link
Member

lzap commented Oct 12, 2016

[test]

@lzap
Copy link
Member

lzap commented Oct 12, 2016

I get some unit failures locally (Jenkins is busted for now):

 1) Error:
DiscoveredHostsControllerTest#test_edit_form_elements:
NoMethodError: undefined method `registered_smart_proxies' for #<Class:0x0055e4d8677ad8>
    app/controllers/concerns/foreman/controller/parameters/smart_proxies_common.rb:6:in `add_smart_proxies_common_params_filter'
    app/controllers/concerns/foreman/controller/parameters/host_common.rb:16:in `add_host_common_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:12:in `block in discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `tap'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:22:in `discovered_host_params_host'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:54:in `edit'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:336:in `skip_bullet'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:52:in `test_edit_form_elements'


  2) Error:
DiscoveredHostsControllerTest#test_dns_rebuild:
NoMethodError: undefined method `registered_smart_proxies' for #<Class:0x0055e4d8677ad8>
    app/controllers/concerns/foreman/controller/parameters/smart_proxies_common.rb:6:in `add_smart_proxies_common_params_filter'
    app/controllers/concerns/foreman/controller/parameters/host_common.rb:16:in `add_host_common_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:12:in `block in discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `tap'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:22:in `discovered_host_params_host'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:68:in `update'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:278:in `test_dns_rebuild'


  3) Error:
DiscoveredHostsControllerTest#test_edit_form_quick_submit:
NoMethodError: undefined method `registered_smart_proxies' for #<Class:0x0055e4d8677ad8>
    app/controllers/concerns/foreman/controller/parameters/smart_proxies_common.rb:6:in `add_smart_proxies_common_params_filter'
    app/controllers/concerns/foreman/controller/parameters/host_common.rb:16:in `add_host_common_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:12:in `block in discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `tap'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:22:in `discovered_host_params_host'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:54:in `edit'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:336:in `skip_bullet'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:72:in `block in test_edit_form_quick_submit'
    test/active_support_test_case_helper.rb:102:in `disable_taxonomies'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:68:in `test_edit_form_quick_submit'


  4) Error:
DiscoveredHostsControllerTest#test_no_dns_rebuild_if_dns_pending:
NoMethodError: undefined method `registered_smart_proxies' for #<Class:0x0055e4d8677ad8>
    app/controllers/concerns/foreman/controller/parameters/smart_proxies_common.rb:6:in `add_smart_proxies_common_params_filter'
    app/controllers/concerns/foreman/controller/parameters/host_common.rb:16:in `add_host_common_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:12:in `block in discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `tap'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:22:in `discovered_host_params_host'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:68:in `update'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:265:in `test_no_dns_rebuild_if_dns_pending'


  5) Error:
DiscoveredHostsControllerTest#test_edit_form_submit_parameters:
NoMethodError: undefined method `registered_smart_proxies' for #<Class:0x0055e4d8677ad8>
    app/controllers/concerns/foreman/controller/parameters/smart_proxies_common.rb:6:in `add_smart_proxies_common_params_filter'
    app/controllers/concerns/foreman/controller/parameters/host_common.rb:16:in `add_host_common_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:12:in `block in discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `tap'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:8:in `discovered_host_params_filter'
    /home/lzap/work/foreman_discovery/app/controllers/concerns/foreman/controller/parameters/discovered_host.rb:22:in `discovered_host_params_host'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:54:in `edit'
    /home/lzap/work/foreman_discovery/app/controllers/discovered_hosts_controller.rb:336:in `skip_bullet'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    /home/lzap/work/foreman_discovery/test/functional/discovered_hosts_controller_test.rb:105:in `test_edit_form_submit_parameters'

@timogoebel
Copy link
Member Author

Ah, yes. Sorry for this. Tests should pass now.

lzap
lzap reviewed Oct 12, 2016
View reviewed changes
app/models/host/discovered.rb
@@ -1,6 +1,9 @@
class Host::Discovered < ::Host::Base
include ScopedSearchExtensions
include Foreman::Renderer
include BelongsToProxies

self.registered_smart_proxies = {}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this, why I don't see it in core? Isn't better to initialize the class member in the BelongsToProxies concern?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean by that is - I don't understand why core models do not need this initialization. Anyway, we should either handle that correctly or initialize it in core and not here, what you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I'll write a fix for core. The concern was only meant to be used on a class which actually has smart_proxies. But we need it here because the params filter expect the method to be defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for reference, the core PR is this: theforeman/foreman#3932

@lzap lzap merged commit 4a31890 into theforeman:develop Oct 12, 2016
@lzap
Copy link
Member

lzap commented Oct 12, 2016

Tests are green locally, thanks @timogoebel for this huge improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzap lzap lzap approved these changes

Assignees

@lzap lzap

Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
3 participants
@timogoebel @lzap @theforeman-bot