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

Fixes #3309 - Support deep merging of hash and array structures in smart class parameters #1715

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/assets/javascripts/lookup_keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,33 @@ function toggleOverrideValue(item) {
override_value_div.toggle(override);
}

function changeCheckboxEnabledStatus(checkbox, shouldEnable) {
if (shouldEnable) {
$(checkbox).attr('disabled', null);
}
else {
$(checkbox).attr('checked', false);
$(checkbox).attr('disabled', 'disabled');
}
}

function keyTypeChange(item) {
var reloadedItem = $(item);
var keyType = reloadedItem.val();
var fields = reloadedItem.closest('.fields');
var mergeOverrides = fields.find("[id$='_merge_overrides']");
var avoidDuplicates = fields.find("[id$='_avoid_duplicates']");
changeCheckboxEnabledStatus(mergeOverrides, keyType == 'array' || keyType == 'hash');
changeCheckboxEnabledStatus(avoidDuplicates, keyType == 'array' && $(mergeOverrides).attr('checked') == 'checked');
}

function mergeOverridesChanged(item) {
var fields = $(item).closest('.fields');
var keyType = fields.find("[id$='_key_type']").val();
var avoidDuplicates = fields.find("[id$='_avoid_duplicates']");
changeCheckboxEnabledStatus(avoidDuplicates, keyType == 'array' && item.checked);
}

function filterByEnvironment(item){
if ($(item).val()=="") {
$('ul.smart-var-tabs li[data-used-environments] a').removeClass('text-muted');
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/api/v2/smart_class_parameters_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ def show
param :override_value_order, String
param :parameter_type, String
param :required, :bool
param :merge_overrides, :bool
param :avoid_duplicates, :bool
end

def update
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/api/v2/smart_variables_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def show
param :validator_type, String
param :validator_rule, String
param :variable_type, String
param :merge_overrides, :bool
param :avoid_duplicates, :bool
end
end

Expand Down
6 changes: 3 additions & 3 deletions app/helpers/lookup_keys_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ def show_puppet_class(f)
end unless @puppetclass # nested smart-vars form in a tab of puppetclass/_form: no edition allowed, and the puppetclass is already visible as a context
end

def param_type_selector(f)
def param_type_selector(f, options = {})
selectable_f f, :key_type, options_for_select(LookupKey::KEY_TYPES.map { |e| [_(e),e] }, f.object.key_type),{},
{ :disabled => (f.object.is_param && !f.object.override), :size => "col-md-8",
options.merge({ :disabled => (f.object.is_param && !f.object.override), :size => "col-md-8",
:help_block => popover(_("Parameter types"),_("<dl>" +
"<dt>String</dt> <dd>Everything is taken as a string.</dd>" +
"<dt>Boolean</dt> <dd>Common representation of boolean values are accepted.</dd>" +
Expand All @@ -53,7 +53,7 @@ def param_type_selector(f)
"<dt>Hash</dt> <dd>A valid JSON or YAML input, that must evaluate to an object/map/dict/hash.</dd>" +
"<dt>YAML</dt> <dd>Any valid YAML input.</dd>" +
"<dt>JSON</dt> <dd>Any valid JSON input.</dd>" +
"</dl>"), :title => _("How values are validated")).html_safe}
"</dl>"), :title => _("How values are validated")).html_safe})
end

def validator_type_selector(f)
Expand Down
24 changes: 23 additions & 1 deletion app/models/lookup_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,16 @@ def audit_class
validates :key_type, :inclusion => {:in => KEY_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true
validate :validate_list, :validate_regexp
validates_associated :lookup_values
validate :ensure_type
validate :ensure_type, :disable_merge_overrides, :disable_avoid_duplicates

before_save :sanitize_path
attr_name :key

scoped_search :on => :key, :complete_value => true, :default_order => true
scoped_search :on => :lookup_values_count, :rename => :values_count
scoped_search :on => :override, :complete_value => {:true => true, :false => false}
scoped_search :on => :merge_overrides, :complete_value => {:true => true, :false => false}
scoped_search :on => :avoid_duplicates, :complete_value => {:true => true, :false => false}
scoped_search :in => :param_classes, :on => :name, :rename => :puppetclass, :complete_value => true
scoped_search :in => :lookup_values, :on => :value, :rename => :value, :complete_value => true

Expand Down Expand Up @@ -97,6 +99,14 @@ def is_smart_class_parameter?
is_param? && environment_classes.any?
end

def supports_merge?
['array', 'hash'].include?(key_type)
end

def supports_uniq?
key_type == 'array'
end

def to_param
"#{id}-#{key.parameterize}"
end
Expand Down Expand Up @@ -279,4 +289,16 @@ def validate_list
errors.add(:default_value, _("%{default_value} is not one of %{validator_rule}") % { :default_value => default_value, :validator_rule => validator_rule }) and return false unless validator_rule.split(KEY_DELM).map(&:strip).include?(default_value)
end

def disable_merge_overrides
if merge_overrides && !supports_merge?
self.errors.add(:merge_overrides, _("can only be set for array or hash"))
end
end

def disable_avoid_duplicates
if avoid_duplicates && (!merge_overrides || !supports_uniq?)
self.errors.add(:avoid_duplicates, _("can only be set for arrays that have merge_overrides set to true"))
end
end

end
105 changes: 83 additions & 22 deletions app/services/classification/base.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
module Classification
class Base

delegate :hostgroup, :environment_id, :puppetclass_ids, :classes,
:to => :host

def initialize(args = { })
def initialize(args = {})
@host = args[:host]
@safe_render = SafeRender.new(:variables => { :host => host } )
@safe_render = SafeRender.new(:variables => {:host => host})
end

#override to return the relevant enc data and format
Expand Down Expand Up @@ -34,23 +35,27 @@ def possible_value_orders
end

def values_hash(options = {})
Copy link
Member

Choose a reason for hiding this comment

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

This method is hard to read (not you fault) and you're adding more complexity. Therefore I'd consider doing some refactorings. First - you can extract values[key_id][name] into some variable so you don't have to repeat it on every line.

Then you can use hash with default values values = Hash.new {|h, k| h[k] = {}; values[:new_key][name] = triplet so you avoid hash[key_id] ||= {} lines.

Furthermore you don't have to store values_for_deep_merge in this complex hash format. I'd suggest using array OpenStruct or Struct instance for keeping value, index, element and element_name. Then calling hash_deep_merge that will generate values in desired hash structure. The benefit is that hash_deep_merge will be much easier to read. Ideally I'd use OpenStruct in whole values_hash method and convert the result into hash at the end (maybe some convert method).

Also I think you should compute values inside when "hash" branch, not looking at values_for_deep_merge outside of hash context in unless condition (this will also eliminate the need of values_for_deep_merge being global to the whole method).

Last but not least, I'd split this method so the actual evaluation is separated per type so we'd have methods like update_array_matcher, update_hash_matcher, update_generic_matcher which can be tested separately.

Let me know if I should elaborate any of previous refactorings. This file would deserves many other improvements but these are not related to the PR.

values = {}
path2matches.each do |match|
LookupValue.where(:match => match).where(:lookup_key_id => class_parameters.map(&:id)).each do |value|
key_id = value.lookup_key_id
values[key_id] ||= {}
key = class_parameters.detect{|k| k.id == value.lookup_key_id }
name = key.to_s
element = match.split(LookupKey::EQ_DELM).first
element_name = match.split(LookupKey::EQ_DELM).last
next if options[:skip_fqdn] && element=="fqdn"
if values[key_id][name].nil?
values[key_id][name] = {:value => value.value, :element => element, :element_name => element_name}
else
if key.path.index(element) < key.path.index(values[key_id][name][:element])
values[key_id][name] = {:value => value.value, :element => element}
end
values = Hash.new { |h,k| h[k] = {} }
all_lookup_values = LookupValue.where(:match => path2matches).where(:lookup_key_id => class_parameters)
class_parameters.each do |key|
lookup_values_for_key = all_lookup_values.where(:lookup_key_id => key.id)
sorted_lookup_values = lookup_values_for_key.sort_by { |lv| key.path.index(lv.match.split(LookupKey::EQ_DELM).first) }
value = nil
if key.merge_overrides
case key.key_type
when "array"
value = update_array_matcher(key.avoid_duplicates, sorted_lookup_values, options)
when "hash"
value = update_hash_matcher(sorted_lookup_values, options)
else
raise "merging enabled for non mergeable key #{key.key}"
end
else
value = update_generic_matcher(sorted_lookup_values, options)
end

if value.present?
values[key.id][key.key] = value
end
end
values
Expand Down Expand Up @@ -91,7 +96,7 @@ def path2matches
matches << match.join(LookupKey::KEY_DELM)

hostgroup_matches.each do |hostgroup_match|
match[match.index{|m|m =~ /hostgroup\s*=/}]=hostgroup_match
match[match.index { |m| m =~ /hostgroup\s*=/ }]=hostgroup_match
matches << match.join(LookupKey::KEY_DELM)
end if Array.wrap(rule).include?("hostgroup") && Setting["host_group_matchers_inheritance"]
end
Expand All @@ -106,7 +111,7 @@ def attr_to_value(element)
# host parameter
return host.host_params[element] if host.host_params.include?(element)
# fact attribute
if (fn = host.fact_names.first(:conditions => { :name => element }))
if (fn = host.fact_names.first(:conditions => {:name => element}))
return FactValue.where(:host_id => host.id, :fact_name_id => fn.id).first.value
end
end
Expand All @@ -118,6 +123,62 @@ def path_elements(path = nil)
end
end
end
end
end

private

def update_generic_matcher(lookup_values, options)
if options[:skip_fqdn]
while lookup_values.present? && lookup_values.first.match.split(LookupKey::EQ_DELM).first == "fqdn"
lookup_values.delete_at(0)
end
end

if lookup_values.present?
lv = lookup_values.first
element, element_name = lv.match.split(LookupKey::EQ_DELM)
{:value => lv.value, :element => element,
:element_name => element_name}
end
end

def update_array_matcher(should_avoid_duplicates, lookup_values, options)
elements = []
values = []
element_names = []

lookup_values.each do |lookup_value|
element, element_name = lookup_value.match.split(LookupKey::EQ_DELM)
next if (options[:skip_fqdn] && element=="fqdn")
elements << element
element_names << element_name
if should_avoid_duplicates
values |= lookup_value.value
else
values += lookup_value.value
end
end

{:value => values, :element => elements,
:element_name => element_names}
end

def update_hash_matcher(lookup_values, options)
elements = []
values = {}
element_names = []

# to make sure seep merge overrides by priority, putting in the values with the lower priority first
# and then merging with higher priority
lookup_values.reverse.each do |lookup_value|
element, element_name = lookup_value.match.split(LookupKey::EQ_DELM)
next if (options[:skip_fqdn] && element=="fqdn")
elements << element
element_names << element_name
values.deep_merge!(lookup_value.value)
end

{:value => values, :element => elements,
:element_name => element_names}
end
end
end
2 changes: 1 addition & 1 deletion app/views/api/v2/smart_class_parameters/main.json.rabl
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ object @smart_class_parameter
extends "api/v2/smart_class_parameters/base"

attributes :description, :override, :parameter_type, :default_value, :required, :validator_type, :validator_rule,
:override_value_order, :override_values_count, :created_at, :updated_at
:merge_overrides, :avoid_duplicates, :override_value_order, :override_values_count, :created_at, :updated_at
2 changes: 1 addition & 1 deletion app/views/api/v2/smart_variables/main.json.rabl
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ object @smart_variable
extends "api/v2/smart_variables/base"

attributes :description, :parameter_type, :default_value, :validator_type, :validator_rule,
:override_value_order, :override_values_count,
:override_value_order, :override_values_count, :merge_overrides, :avoid_duplicates,
:puppetclass_id, :puppetclass_name, :created_at, :updated_at
11 changes: 8 additions & 3 deletions app/views/lookup_keys/_fields.html.erb
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
<div id='<%= (f.object.key || 'new_lookup_keys').to_s.gsub(' ','_') + '_' + f.object.id.to_s %>' class='tab-pane fields' >
<% is_param = f.object.is_param %>

<%= text_f(f, :environment_classes, :value => f.object.environment_classes.map(&:environment).to_sentence, :label=> _('Puppet Environments'),:size => "col-md-8", :disabled=>true) if is_param%>
<%= text_f(f, :environment_classes, :value => f.object.environment_classes.map(&:environment).to_sentence, :label => _('Puppet Environments'),:size => "col-md-8", :disabled=>true) if is_param%>

<%= remove_child_link(_("Remove %s?") % (f.object.new_record? ? _("Variable") : f.object), f , {:class => 'btn btn-danger hide'}) unless controller_name == "lookup_keys" %>
<%= text_f f, :key, :disabled => f.object.is_param, :size => "col-md-8" %>
<%= f.hidden_field :key if is_param %>
<%= textarea_f f, :description, :rows => :auto, :size => "col-md-8" %>

<%= show_puppet_class f %>
<%= checkbox_f(f, :override, :onchange=>'toggleOverrideValue(this)', :size => "col-md-8",
<%= checkbox_f(f, :override, :onchange => 'toggleOverrideValue(this)', :size => "col-md-8",
:help_block => _("Whether the smart-variable should override the Puppet class default value.")
) if is_param%>

<%= param_type_selector f %>
<%= param_type_selector(f, :onchange => 'keyTypeChange(this)') %>
<%= textarea_f f, :default_value, :value => f.object.default_value_before_type_cast,:size => "col-md-8", :disabled => (f.object.is_param && !f.object.override), :rows => :auto,
:help_block => _("Value to use when there is no match") %>
<div <%= "id=#{(f.object.key || 'new_lookup_keys').to_s.gsub(' ','_')}_lookup_key_override_value" %> style=<%= "display:none;" if (f.object.is_param && !f.object.override) %>>
Expand All @@ -25,6 +25,11 @@
<%= text_f f, :validator_rule, :size => "col-md-8", :disabled => f.object.validator_type.blank? %>

<legend><%= _("Override value for specific hosts") %></legend>
<%= checkbox_f(f, :merge_overrides, :onchange => 'mergeOverridesChanged(this)',
:disabled => !f.object.supports_merge?, :size => "col-md-8",
:help_block => _("Should the matchers continue to look for matches after first find (only array/hash type).")) %>
<%= checkbox_f(f, :avoid_duplicates, :disabled => (!f.object.supports_uniq? || !f.object.merge_overrides), :size => "col-md-8",
:help_block => _("Should the matched result avoid duplicate values (only array type).")) %>
<%= textarea_f f, :path, :rows => :auto, :label => _("Order"), :size => "col-md-8", :value => f.object.path,
:help_block => popover(_("The order in which values are resolved"),
_("The order in which matchers keys are processed, first match wins.<br> You may use multiple attributes as a matcher key, for example, an order of <code>host group, environment</code> would expect a matcher such as <code>hostgroup = \"web servers\", environment = production</code>"), :title => _("The order in which values are resolved")).html_safe
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddMergeOverridesAndAvoidDuplicatesToLookupKey < ActiveRecord::Migration
def change
add_column :lookup_keys, :merge_overrides, :boolean
add_column :lookup_keys, :avoid_duplicates, :boolean
end
end
1 change: 1 addition & 0 deletions test/fixtures/environment_classes.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,4 @@ nine:
puppetclass: three
environment: global_puppetmaster
lookup_key_id: nil

Loading