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 #15406 - Moved puppet outside hosts helper #3607

Merged
merged 1 commit into from
Aug 28, 2019
Merged

Fixes #15406 - Moved puppet outside hosts helper #3607

merged 1 commit into from
Aug 28, 2019

Conversation

ShimShtein
Copy link
Member

In order to make lists of items more extendable, I have changed the way our helpers generate lists of actions/fields.

  1. Created a registry that will store a list of methods that are contributing to a each placeholder
  2. Modified each item in the list to contain a priority field, so the items could be sorted differently than the order they are added to the list.
  3. Kept backwards (plugins) compatibility as much as possible.

Central registry

Added a singleton class instance that will be used as providers registry - each entry in this registry is a symbol that point to a method that will provide the relevant list items.
Now a helper can declare at module level which methods should be used from that module to provide different items.
Usage example:

module PuppetRelatedHelper
  UI.host_description.multiple_actions_provider :puppet_actions
  UI.host_description.overview_fields_provider :puppet_host_overview_fields

  def puppet_actions
  # ... snip ...
  end

  def puppet_host_overview_fields(host)
  # ... snip ...
  end

Files: app/services/ui.rb and app/services/ui/host_description.rb.

Priority field

Most of our items lists like multiple_actions, overview_fields e.t.c. are using array of elements to declare the elements that will be used on different host screens. I have modified each item to be a hash that contains two fields: :action or :field that will contain the original item and :priority that will contain the priority of that item.
Instead of:

      actions << [_('Change Puppet Master'), select_multiple_puppet_proxy_hosts_path] if SmartProxy.unscoped.authorized.with_features("Puppet").exists?

The code looks like:

      actions << { :action => [_('Change Puppet Master'), select_multiple_puppet_proxy_hosts_path], :priority => 1050 } if SmartProxy.unscoped.authorized.with_features("Puppet").exists?

Backwards (plugin) compatibility

The methods didn't change their name. Which means that other plugins still can alias_method_chain to add their items. Methods that implement sorting are aware of the fact that part of the items will be in old format.

@xprazak2
Copy link
Contributor

Could you rebase pls?

houndci-bot
houndci-bot reviewed Jun 18, 2017
View reviewed changes
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jun 18, 2017
View reviewed changes
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jun 18, 2017
View reviewed changes
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jun 18, 2017
View reviewed changes
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
app/helpers/hosts_helper.rb Outdated Show resolved Hide resolved
@ShimShtein ShimShtein changed the title Fixes #15406 - Moved puppet outside hosts helper [WIP] Fixes #15406 - Moved puppet outside hosts helper Jun 18, 2017
@ShimShtein
Copy link
Member Author

Fixing rubocop, codeclimate and registry...

@xprazak2
Copy link
Contributor

It works as expected for me, extra points for the backward compatibility.

@ares
Copy link
Member

ares commented Jun 30, 2017

setting as WoC because of WIP in the subject, @ShimShtein what's missing? do you plan to send PRs to plugins so they can start using the new registry?

@ShimShtein
Copy link
Member Author

I am fixing the registry to our latest guidelines, will get back to this PR soon. My first intention was separating puppet actions/fields, but if you have other plugins that need this in your mind, I will submit a PR for them too.

@mmoll
Copy link
Contributor

mmoll commented Jan 7, 2018

@ShimShtein what's the status here?

@tbrisker tbrisker added the WIP label Jul 15, 2018
@xprazak2
Copy link
Contributor

xprazak2 commented Apr 1, 2019

@ShimShtein, this will need a rebase.

@ShimShtein ShimShtein force-pushed the 15406 branch 3 times, most recently from 719d545 to fc57cf6 Compare April 7, 2019 15:32
@ShimShtein ShimShtein changed the title [WIP] Fixes #15406 - Moved puppet outside hosts helper Fixes #15406 - Moved puppet outside hosts helper Apr 7, 2019
@ShimShtein
Copy link
Member Author

[test foreman]

@ShimShtein
Copy link
Member Author

@xprazak2, Rebased. ready for review

@ShimShtein ShimShtein removed the WIP label Apr 7, 2019
@mmoll
Copy link
Contributor

mmoll commented Apr 7, 2019

@ShimShtein I think the integration test failure is related

@theforeman-bot
Copy link
Member

@ShimShtein, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

houndci-bot
houndci-bot reviewed Aug 21, 2019
View reviewed changes
app/helpers/puppet_related_helper.rb Outdated
_("Puppet Environment"),
link_to(host.environment, hosts_path(:search => "environment = #{host.environment}"))
],
:priority => 650
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

app/helpers/puppet_related_helper.rb Outdated
fields << {
:field => [
_("Puppet Environment"),
link_to(host.environment, hosts_path(:search => "environment = #{host.environment}"))
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
[
{ :button => link_to_if_authorized(_("Audits"), hash_for_host_audits_path(:host_id => @host), :title => _("Host audit entries"), :class => 'btn btn-default'), :priority => 100 },
({ :button => link_to_if_authorized(_("Facts"), hash_for_host_facts_path(:host_id => host), :title => _("Browse host facts"), :class => 'btn btn-default'), :priority => 200 } if host.fact_values.any?),
({ :button => link_to_if_authorized(_("Reports"), hash_for_host_config_reports_path(:host_id => host), :title => _("Browse host config management reports"), :class => 'btn btn-default'), :priority => 300 } if host.reports.any?)
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
:method => :delete)
),
:priority => 300
}
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
:data => { :message => delete_host_dialog(host) },
:method => :delete)
),
:priority => 300
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

app/helpers/host_description_helper.rb Outdated
{ :field => [
_(status.name),
content_tag(:span, ' '.html_safe, :class => host_global_status_icon_class(status.to_global)) +
content_tag(:span, _(status.to_label), :class => host_global_status_class(status.to_global))
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
content_tag(:span, _(global_status.to_label), :class => host_global_status_class(global_status.status))
],
:priority => 10
}
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
content_tag(:span, ''.html_safe, :class => host_global_status_icon_class(global_status.status)) +
content_tag(:span, _(global_status.to_label), :class => host_global_status_class(global_status.status))
],
:priority => 10
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

app/helpers/host_description_helper.rb Outdated
:field => [
_("Status"),
content_tag(:span, ''.html_safe, :class => host_global_status_icon_class(global_status.status)) +
content_tag(:span, _(global_status.to_label), :class => host_global_status_class(global_status.status))
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

app/helpers/host_description_helper.rb Outdated
{ :action => [_('Disable Notifications'), multiple_disable_hosts_path], :priority => 400 },
{ :action => [_('Enable Notifications'), multiple_enable_hosts_path], :priority => 500 },
{ :action => [_('Disassociate Hosts'), multiple_disassociate_hosts_path], :priority => 600 },
{ :action => [_('Rebuild Config'), rebuild_config_hosts_path], :priority => 700 }
Copy link

Choose a reason for hiding this comment

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

Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.

@ShimShtein
Copy link
Member Author

@xprazak2, addressed everything (I think).

@xprazak2
Copy link
Contributor

Still getting a couple of errors on show page, uptime is causing problems:

uptime-error

Items from plugins added in the old fashion cause problems for sorting by priority:

prio-sorting

Could you also add the example to the developer docs?

@ShimShtein
Copy link
Member Author

@xprazak2, found both bugs (one definition change that I have not noticed and one method that escaped deletion at merge) and added documentation.

@mmoll
Copy link
Contributor

mmoll commented Aug 26, 2019

this needs a rebase

@xprazak2
Copy link
Contributor

Works well, one final rebase and we can merge.

@ShimShtein
Copy link
Member Author

@xprazak2, @mmoll, rebased. Looks good in tests so far too.

@xprazak2 xprazak2 merged commit 377ff66 into theforeman:develop Aug 28, 2019
@xprazak2
Copy link
Contributor

Thanks!

@timogoebel timogoebel mentioned this pull request Aug 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@xprazak2 xprazak2 xprazak2 approved these changes

Assignees

@xprazak2 xprazak2

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@ShimShtein @xprazak2 @ares @mmoll @theforeman-bot @houndci-bot @tbrisker